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

[howlbot-integration]

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

[khangvv]

This aligns with the native design of the AggregateRouter (or its inherited implementation from Uniswap), where users are required to pre-deposit the amount before executing the wrap command. And to clarify, no value is stored in the contract that could be drained. Any minor dust amounts should be freely claimable.

[alex-ppg]

The Warden claims that the ETH wrapping operation of the Dispatch contract does not ensure adequate msg.value has been supplied to it. This validation is not meant to be applied to it as the Dispatch contract is an "ephemeral" implementation, meaning that it is never meant to hold funds at rest and is simply meant to process them.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid