Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unvalidated msg.value in AggregateRouter::execute() Allows Unauthorized WRAP_ETH and UNWRAP_WETH Operations, Potentially Leading to Contract Fund Drain #208

Open
c4-bot-10 opened this issue Oct 30, 2024 · 1 comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working 🤖_50_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality

Comments

@c4-bot-10
Copy link
Contributor

Lines of code

https://github.com/ronin-chain/katana-operation-contracts/blob/27f9d28e00958bf3494fa405a8a5acdcd5ecdc5d/src/aggregate-router/AggregateRouter.sol#L37
https://github.com/ronin-chain/katana-operation-contracts/blob/27f9d28e00958bf3494fa405a8a5acdcd5ecdc5d/src/aggregate-router/base/Dispatcher.sol#L164-L180

Vulnerability details

Summary

A vulnerability exists in the AggregateRouter::execute() function when handling the Commands.WRAP_ETH command on the Dipacther.sol. This vulnerability allows an attacker to attempt wrapping Ether without sending the corresponding amount of Ether (msg.value), potentially leading to unexpected behavior or financial loss.

Vulnerability Details

The dispatch function within the Dispatcher contract, specifically when processing the Commands.WRAP_ETH command.

function dispatch(bytes1 commandType, bytes calldata inputs) internal returns (bool success, bytes memory output) {
    uint256 command = uint8(commandType & Commands.COMMAND_TYPE_MASK);
    //... 
    // @audit msg.value not checked
    else if (command == Commands.WRAP_ETH) {
        // equivalent: abi.decode(inputs, (address, uint256))
        address recipient;
        uint256 amountMin;
        assembly {
          recipient := calldataload(inputs.offset)
          amountMin := calldataload(add(inputs.offset, 0x20))
        }
        Payments.wrapETH(map(recipient), amountMin);
}
  function wrapETH(address recipient, uint256 amount) internal {
    if (amount == Constants.CONTRACT_BALANCE) {
      amount = address(this).balance;
    } else if (amount > address(this).balance) {
      revert InsufficientETH();
    }
    if (amount > 0) {
      WETH9.deposit{ value: amount }();
      if (recipient != address(this)) {
        WETH9.transfer(recipient, amount);
      }
    }
  }

The attacker prepares the WRAP_ETH command with a fake amount.
The attacker can call the execute function without sending the required Ether (msg.value). Since the dispatch function does not validate msg.value, the wrapETH function could be executed with an incorrect amount, potentially leading to unintended WETH transfers. A similar issue exists with the UNWRAP_WETH command, which could lead to the contract's funds being completely drained.

Impact

this could lead to incorrect Ether wrapping, resulting in financial discrepancies or loss.

Tools Used

Manual Review

Recommendations

Add a check to ensure that the amount to be wrapped does not exceed msg.value.

Use msg.value directly if applicable, to ensure consistency.

Assessed type

ETH-Transfer

@c4-bot-10 c4-bot-10 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Oct 30, 2024
c4-bot-10 added a commit that referenced this issue Oct 30, 2024
@c4-bot-12 c4-bot-12 added the 🤖_50_group AI based duplicate group recommendation label Oct 30, 2024
@howlbot-integration howlbot-integration bot added the sufficient quality report This report is of sufficient quality label Nov 4, 2024
@nevillehuang
Copy link

Likely QA/informational, multicall with sweep/pay can likely be utilized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working 🤖_50_group AI based duplicate group recommendation sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants