Functions in AggregateRouter dont allow reverts

[howlbot-integration]

Lines of code

https://github.com/ronin-chain/katana-operation-contracts/blob/27f9d28e00958bf3494fa405a8a5acdcd5ecdc5d/src/aggregate-router/base/Dispatcher.sol#L31-L51

Vulnerability details

Proof of Concept

The AggregateRouter contract allows functions to fail and still have the other functions succeed. This is implemented via a failure check implemented in the code.

if (!success && successRequired(command)) {
        revert ExecutionFailed({ commandIndex: commandIndex, message: output });
      }

function successRequired(bytes1 command) internal pure returns (bool) {
    return command & Commands.FLAG_ALLOW_REVERT == 0;
  }

So a user can decide to allow a function call to fail, by setting a particular bit in the command.

The command is passed in as a byte array, where each byte represents a single command.
Of that 1 byte, 2 masks are used to define the operation.

A COMMAND_TYPE_MASK of 0x3f (00111111) is used to define the operation. This is the last 6 bits of the byte. A FLAG_ALLOW_REVERT of 0x80 (10000000) is used to define the allow revert. This is the first bit of the byte.

So a user can presumable set an operation with the last 6 bits, and then set an allowrevert in the first bit, to allow execution of other functions. These two things are independent, and can be chosen by the user.

The issue is that the allowRevert functionality mostly doesn't work.

This is because in the Dispatcher.sol contract, the success value is updated in only one function out of the 14 implemented, and none of the function calls use try-catch pattern to check for success.

The success value is set in the contract to true, and then never updated.

    success = true;


    // 0x00 <= command < 0x08
    if (command < Commands.FIRST_IF_BOUNDARY) {
      if (command == Commands.V3_SWAP_EXACT_IN) {
        // Reading inputs
        //@audit no try catch used. Reverts on failure and completely halts execution.
        v3SwapExactInput(map(recipient), amountIn, amountOutMin, path, payer);

As seen in the snippet above, the swap function is called directly, so any reverts in the swap function reverts the main execute function, without giving any opportunity for a graceful failure.

Thus the whole mechanism of using the first bit to define FLAG_ALLOW_REVERT has been ignored.

Only the implementation of the BALANCE_CHECK_ERC20 command updates success correctly.

success = (ERC20(token).balanceOf(owner) >= minBalance);
if (!success) output = abi.encodePacked(BalanceTooLow.selector);

Thus the dispatcher contract does not allow for any soft failures. Since the devs intend to allow for soft failures as shown by the usage of the FLAG_ALLOW_REVERT mask, this is an issue.

Recommended Mitigation Steps

The function calls in the dispatcher should be done with try-catch statements. If failed, the success value should be updated accordingly and returned.

Assessed type

Other

[thaixuandang]

Allowing reverts was initially intended mainly for commands related to NFTs, while commands related to swaps were always set to disallow reverts. Since we’ve removed all NFT-related commands, the only remaining command that uses the allow-revert option is BALANCE_CHECK_ERC20. This doesn’t cause any issues or incompatibility with the SDK, as Uniswap’s Universal Router behaves the same way.

[khangvv]

Thank you, Dang. This should be considered a minor QA issue (inherited from Uniswap) or possibly out of scope.

[alex-ppg]

The Warden outlines that soft-failure functionality in the system is seemingly unused as it is implemented by one out of the functions supported in the system.

A feature being present in only one function does not render it invalid, and the feature itself has been correctly implemented (unless proven otherwise). As such, I believe that this submission is invalid as it outlines the desirable behavior of the code.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid