code-423n4 / 2023-05-maia-findings

24 stars 13 forks source link

Incorrect gas unit calculation in RootBridgeAgent which could possibly lead to a slow draining of execution budget or failure of anyExecute call #601

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L798-L824

Vulnerability details

Impact

Context:

anyExecute method is called by Anycall Executor on the destination chain to execute interaction. The user has to pay for the remote call execution gas, this is done at the end of the call. However, if there is not enough available gas, the anyExecute will be reverted due to a revert caused by the Anycall Executor.

Here is the calculation for the gas used

    //Get Available Gas
    uint256 availableGas = _depositedGas - _gasToBridgeOut;

    //Get Root Environment Execution Cost
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());

    //Check if sufficient balance
    if (minExecCost > availableGas) {
        _forceRevert();
        return;
    }

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/RootBridgeAgent.sol#L798-L824

_forceRevert will withdraw all execution budget.

    // Withdraw all execution gas budget from anycall for tx to revert with "no enough budget"
    if (executionBudget > 0) try anycallConfig.withdraw(executionBudget) {} catch {}

So Anycall Executor will revert if there is not enough budget. This is done at

    uint256 budget = executionBudget[_from];
    require(budget > totalCost, "no enough budget");
    executionBudget[_from] = budget - totalCost;

https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Config.sol#L206C42-L206C58

(1) Gas Calculation:

To calculate how much the user has to pay, the following formula is used:

    //Get Root Environment Execution Cost
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());

Gas units are calculated as follows:

This overhead is supposed to cover:

The issue is that 55_000 is not enough to cover pre 1st gas checkpoint and post last gas checkpoint. This means that the user paying less than the actual gas cost. According to the sponsor, Bridge Agent deployer deposits first time into anycallConfig where the goal is to replenish the execution budget after use every time. The issue could possibly lead to:

  1. execution budget is decreasing over time (slow draining) in case it has funds already.
  2. anyExecute calls will fail since the calculation of the gas used in the Anycall contracts is way bigger. In Anycall, this is done by the modifier chargeDestFee

(3) Gas Calculation in AnyCall:

There is also a gas consumption at anyExec method called by the MPC (in AnyCall) here

    function anyExec(
        address _to,
        bytes calldata _data,
        string calldata _appID,
        RequestContext calldata _ctx,
        bytes calldata _extdata
    )
        external
        virtual
        lock
        whenNotPaused
        chargeDestFee(_to, _ctx.flags) // <= starting from here
        onlyMPC
    {
        .
        .
        .
        bool success = _execute(_to, _data, _ctx, _extdata);
        .
        .
   }

https://github.com/anyswap/multichain-smart-contracts/blob/main/contracts/anycall/v7/AnycallV7Upgradeable.sol#L276

The gas is nearly 110_000. It is not taken into account.

(3) Base Fee & Input Data Fee:

From Ethereum yellow paper:

Gtransaction 21000 Paid for every transaction Gtxdatazero 4 Paid for every zero byte of data or code for a transaction. Gtxdatanonzero 16 Paid for every non-zero byte of data or code for a transaction.

So

  1. We have 21_000 as a base fee. This should be taken into account. However, it is paid by AnyCall, since the TX is sent by MPC. So, we are fine here. Probably this explains the overhead (100_000) added by anycall

  2. Because anyExecute method has bytes data to be passed, we have extra gas consumption which is not taken into account. For every zero byte => 4 For every non-zero byte => 16

So generally speaking, the bigger the data is, the bigger the gas becomes. you can simply prove this by adding arbitrary data to anyExecute method in PoC#1 test below. and you will see the gas spent increases.

Summary

  1. MIN_EXECUTION_OVERHEAD is underestimated.
  2. The gas consumed by anyExec method called by the MPC is not considered.
  3. Input data fee isn't taken into account.

There are two PoCs proving the first two points above. The third point can be proven by simply adding arbitrary data to anyExecute method in PoC#1 test.

Proof of Concept

PoC#1 (MIN_EXECUTION_OVERHEAD is underestimated)

Overview

This PoC is independent from the codebase (but uses the same code). There are two contracts simulating RootBridgeAgent.anyExecute.

  1. BridgeAgent which has the code of pre 1st gas checkpoint and post last gas checkpoint.
  2. BridgeAgentEmpty which has the code of pre 1st gas checkpoint and post last gas checkpoint commented out.

We run the same test for both, the difference in gas is what’s at least nearly the minimum required to cover pre 1st gas checkpoint and post last gas checkpoint. In this case here it is 76987 which is bigger than 55_000.

Here is the output of the test:

[PASS] test_calcgas() (gas: 213990)
Logs:
  bridgeAgentEmpty.anyExecute Gas Spent => 24572
  bridgeAgent.anyExecute Gas Spent => 101559
  The gas difference 101559 - 24572 = 76987

Explanation

RootBridgeAgent.anyExecute method depends on the following external calls:

  1. AnycallExecutor.context()
  2. AnycallProxy.config()
  3. AnycallConfig.executionBudget()
  4. AnycallConfig.withdraw()
  5. AnycallConfig.deposit()
  6. WETH9.withdraw()

For this reason, I've copied the same code from multichain-smart-contracts. For WETH9, I've used the contract from the codebase which has minimal code.

Please note that:

The coded PoC


### PoC#2 (The gas consumed by `anyExec` method in AnyCall)

#### Overview

We have contracts that simulating Anycall contracts:
1. AnycallV7Config
2. AnycallExecutor
3. AnycallV7

The flow like this:
MPC => AnycallV7 => AnycallExecutor => IApp

In the code, `IApp(_to).anyExecute`  is commented out because we don't want to calculate its gas since it is done in PoC#1.

Here is the output of the test:

```sh
[PASS] test_gasInanycallv7() (gas: 102613)
Logs:
  anycallV7.anyExec Gas Spent => 110893

coded PoC

import {Test} from "forge-std/Test.sol"; import "forge-std/console.sol";

import {DSTest} from "ds-test/test.sol";

/// IAnycallConfig interface of the anycall config interface IAnycallConfig { function checkCall( address _sender, bytes calldata _data, uint256 _toChainID, uint256 _flags ) external view returns (string memory _appID, uint256 _srcFees);

function checkExec(
    string calldata _appID,
    address _from,
    address _to
) external view;

function chargeFeeOnDestChain(address _from, uint256 _prevGasLeft) external;

}

/// IAnycallExecutor interface of the anycall executor interface IAnycallExecutor { function context() external view returns (address from, uint256 fromChainID, uint256 nonce);

function execute(
    address _to,
    bytes calldata _data,
    address _from,
    uint256 _fromChainID,
    uint256 _nonce,
    uint256 _flags,
    bytes calldata _extdata
) external returns (bool success, bytes memory result);

}

/// IApp interface of the application interface IApp { /// (required) call on the destination chain to exec the interaction function anyExecute(bytes calldata _data) external returns (bool success, bytes memory result);

/// (optional,advised) call back on the originating chain if the cross chain interaction fails
/// `_data` is the orignal interaction arguments exec on the destination chain
function anyFallback(bytes calldata _data)
    external
    returns (bool success, bytes memory result);

}

library AnycallFlags { // call flags which can be specified by user uint256 public constant FLAG_NONE = 0x0; uint256 public constant FLAG_MERGE_CONFIG_FLAGS = 0x1; uint256 public constant FLAG_PAY_FEE_ON_DEST = 0x1 << 1; uint256 public constant FLAG_ALLOW_FALLBACK = 0x1 << 2;

// exec flags used internally
uint256 public constant FLAG_EXEC_START_VALUE = 0x1 << 16;
uint256 public constant FLAG_EXEC_FALLBACK = 0x1 << 16;

}

contract AnycallV7Config { uint256 public constant PERMISSIONLESS_MODE = 0x1; uint256 public constant FREE_MODE = 0x1 << 1;

mapping(string => mapping(address => bool)) public appExecWhitelist;
mapping(string => bool) public appBlacklist;

uint256 public mode;

uint256 public minReserveBudget;

mapping(address => uint256) public executionBudget;

constructor() {
     mode = PERMISSIONLESS_MODE;
}

function checkExec(
    string calldata _appID,
    address _from,
    address _to
) external view {
    require(!appBlacklist[_appID], "blacklist");

    if (!_isSet(mode, PERMISSIONLESS_MODE)) {
        require(appExecWhitelist[_appID][_to], "no permission");
    }

    if (!_isSet(mode, FREE_MODE)) {
        require(
            executionBudget[_from] >= minReserveBudget,
            "less than min budget"
        );
    }
}

function _isSet(
    uint256 _value,
    uint256 _testBits
) internal pure returns (bool) {
    return (_value & _testBits) == _testBits;
}

}

contract AnycallExecutor { bytes32 public constant PAUSE_ALL_ROLE = 0x00;

event Paused(bytes32 role);
event Unpaused(bytes32 role);

modifier whenNotPaused(bytes32 role) {
    require(
        !paused(role) && !paused(PAUSE_ALL_ROLE),
        "PausableControl: paused"
    );
    _;
}
mapping(bytes32 => bool) private _pausedRoles;
mapping(address => bool) public isSupportedCaller;

struct Context {
    address from;
    uint256 fromChainID;
    uint256 nonce;
}
// Context public override context;
Context public context;

function paused(bytes32 role) public view virtual returns (bool) {
    return _pausedRoles[role];
}

modifier onlyAuth() {
    require(isSupportedCaller[msg.sender], "not supported caller");
    _;
}

constructor(address anycall) {
    context.fromChainID = 1;
    context.from = address(2);
    context.nonce = 1;

    isSupportedCaller[anycall] = true;
}

function _isSet(uint256 _value, uint256 _testBits) internal pure returns (bool) { return (_value & _testBits) == _testBits; }

// @dev `_extdata` content is implementation based in each version
function execute(
    address _to,
    bytes calldata _data,
    address _from,
    uint256 _fromChainID,
    uint256 _nonce,
    uint256 _flags,
    bytes calldata /*_extdata*/
)
    external
    virtual
    onlyAuth
    whenNotPaused(PAUSE_ALL_ROLE)
    returns (bool success, bytes memory result)
{
    bool isFallback = _isSet(_flags, AnycallFlags.FLAG_EXEC_FALLBACK);

    context = Context({
        from: _from,
        fromChainID: _fromChainID,
        nonce: _nonce
    });

    if (!isFallback) {
        // we skip calling anyExecute since it is irrelevant for this PoC
        // (success, result) = IApp(_to).anyExecute(_data);
    } else {
        (success, result) = IApp(_to).anyFallback(_data);
    }

    context = Context({from: address(0), fromChainID: 0, nonce: 0});
}

}

contract AnycallV7 {

event LogAnyCall(
    address indexed from,
    address to,
    bytes data,
    uint256 toChainID,
    uint256 flags,
    string appID,
    uint256 nonce,
    bytes extdata
);
event LogAnyCall(
    address indexed from,
    string to,
    bytes data,
    uint256 toChainID,
    uint256 flags,
    string appID,
    uint256 nonce,
    bytes extdata
);

event LogAnyExec(
    bytes32 indexed txhash,
    address indexed from,
    address indexed to,
    uint256 fromChainID,
    uint256 nonce,
    bool success,
    bytes result
);

event StoreRetryExecRecord(
    bytes32 indexed txhash,
    address indexed from,
    address indexed to,
    uint256 fromChainID,
    uint256 nonce,
    bytes data
);

// Context of the request on originating chain
struct RequestContext {
    bytes32 txhash;
    address from;
    uint256 fromChainID;
    uint256 nonce;
    uint256 flags;
}
address public mpc;

bool public paused;

// applications should give permission to this executor
address public executor;

// anycall config contract
address public config;

mapping(bytes32 => bytes32) public retryExecRecords;
bool public retryWithPermit;

mapping(bytes32 => bool) public execCompleted;
uint256 nonce;

uint256 private unlocked;

modifier lock() {
    require(unlocked == 1, "locked");
    unlocked = 0;
    _;
    unlocked = 1;
}
/// @dev Access control function
modifier onlyMPC() {
    require(msg.sender == mpc, "only MPC");
    _;
}

/// @dev pausable control function
modifier whenNotPaused() {
    require(!paused, "paused");
    _;
}

function _isSet(uint256 _value, uint256 _testBits)
    internal
    pure
    returns (bool)
{
    return (_value & _testBits) == _testBits;
}

/// @dev Charge an account for execution costs on this chain
/// @param _from The account to charge for execution costs
modifier chargeDestFee(address _from, uint256 _flags) {
    if (_isSet(_flags, AnycallFlags.FLAG_PAY_FEE_ON_DEST)) {
        uint256 _prevGasLeft = gasleft();
        _;
        IAnycallConfig(config).chargeFeeOnDestChain(_from, _prevGasLeft);
    } else {
        _;
    }
}

constructor(address _mpc) {
    unlocked = 1; // needs to be unlocked initially
    mpc = _mpc;
    config = address(new AnycallV7Config());
    executor = address(new AnycallExecutor(address(this)));
}

/// @notice Calc unique ID
function calcUniqID(
    bytes32 _txhash,
    address _from,
    uint256 _fromChainID,
    uint256 _nonce
) public pure returns (bytes32) {
    return keccak256(abi.encode(_txhash, _from, _fromChainID, _nonce));
}

function _execute(
    address _to,
    bytes memory _data,
    RequestContext memory _ctx,
    bytes memory _extdata
) internal returns (bool success) {
    bytes memory result;

    try
        IAnycallExecutor(executor).execute(
            _to,
            _data,
            _ctx.from,
            _ctx.fromChainID,
            _ctx.nonce,
            _ctx.flags,
            _extdata
        )
    returns (bool succ, bytes memory res) {
        (success, result) = (succ, res);
    } catch Error(string memory reason) {
        result = bytes(reason);
    } catch (bytes memory reason) {
        result = reason;
    }

    emit LogAnyExec(
        _ctx.txhash,
        _ctx.from,
        _to,
        _ctx.fromChainID,
        _ctx.nonce,
        success,
        result
    );
}

/**
    @notice Execute a cross chain interaction
    @dev Only callable by the MPC
    @param _to The cross chain interaction target
    @param _data The calldata supplied for interacting with target
    @param _appID The app identifier to check whitelist
    @param _ctx The context of the request on originating chain
    @param _extdata The extension data for execute context
*/

// Note: changed from callback to memory so we can call it from the test contract function anyExec( address _to, bytes memory _data, string memory _appID, RequestContext memory _ctx, bytes memory _extdata ) external virtual lock whenNotPaused chargeDestFee(_to, _ctx.flags) onlyMPC { IAnycallConfig(config).checkExec(_appID, _ctx.from, _to); bytes32 uniqID = calcUniqID( _ctx.txhash, _ctx.from, _ctx.fromChainID, _ctx.nonce ); require(!execCompleted[uniqID], "exec completed");

    bool success = _execute(_to, _data, _ctx, _extdata);
    // success = false on purpose, because when it is true, it consumes less gas. so we are considering worse case here

    // set exec completed (dont care success status)
    execCompleted[uniqID] = true;

    if (!success) {
        if (_isSet(_ctx.flags, AnycallFlags.FLAG_ALLOW_FALLBACK)) { 
            // this will be executed here since the call failed
            // Call the fallback on the originating chain
            nonce++;
            string memory appID = _appID; // fix Stack too deep
            emit LogAnyCall(
                _to,
                _ctx.from,
                _data,
                _ctx.fromChainID,
                AnycallFlags.FLAG_EXEC_FALLBACK |
                    AnycallFlags.FLAG_PAY_FEE_ON_DEST, // pay fee on dest chain
                appID,
                nonce,
                ""
            );
        } else {
            // Store retry record and emit a log
            bytes memory data = _data; // fix Stack too deep
            retryExecRecords[uniqID] = keccak256(abi.encode(_to, data));
            emit StoreRetryExecRecord(
                _ctx.txhash,
                _ctx.from,
                _to,
                _ctx.fromChainID,
                _ctx.nonce,
                data
            );
        }
    }
}

}

contract GasCalcAnyCallv7 is DSTest, Test {

AnycallV7 anycallV7;
address mpc = vm.addr(7);
function setUp() public {
    anycallV7 = new AnycallV7(mpc);
}

function test_gasInanycallv7() public {

    vm.prank(mpc);
    AnycallV7.RequestContext memory ctx = AnycallV7.RequestContext({
        txhash:keccak256(""),
        from:address(0),
        fromChainID:1,
        nonce:1,
        flags:AnycallFlags.FLAG_ALLOW_FALLBACK
    });
    uint256 gasStart_ = gasleft();
    anycallV7.anyExec(address(0),bytes(""),"1",ctx,bytes(""));
    uint256 gasEnd_ = gasleft();
    vm.stopPrank();
    uint256 gasSpent_ = gasStart_ - gasEnd_;
    console.log("anycallV7.anyExec Gas Spent => %d", gasSpent_);
}

}



## Tools Used
Manual analysis

## Recommended Mitigation Steps

Increase the MIN_EXECUTION_OVERHEAD by:
- 35_000 for `RootBridgeAgent.anyExecute`.
- 110_000 for `anyExec` method in AnyCall.

35_000 + 110_000 = 145_000
So MIN_EXECUTION_OVERHEAD becomes 300_000 instead of 155_000. 

Additionally, calculate the gas consumption of the input data passed, add it to the cost.

Note: I suggest that the MIN_EXECUTION_OVERHEAD should be configurable/changeable. After launching OmniChain for some time, collect stats about the actual gas used for AnyCall on chain then adjust it accordingly. This also keeps you in the safe side in case any changes are applied on AnyCall contracts in future since it is upgradable.

## Assessed type

Other
c4-judge commented 1 year ago

trust1995 marked the issue as primary issue

c4-judge commented 1 year ago

trust1995 marked the issue as satisfactory

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor confirmed

c4-sponsor commented 1 year ago

0xBugsy marked the issue as disagree with severity

0xBugsy commented 1 year ago

The variable data cost should be addressed by consulting premium(), the value is used in their calcualtions here uint256 totalCost = gasUsed * (tx.gasprice + _feeData.premium); and we should abide and only pay as much as they will credit us

trust1995 commented 1 year ago

@0xBugsy pls share reasoning for reduced severity. Thanks.

c4-judge commented 1 year ago

trust1995 marked the issue as selected for report

0xBugsy commented 1 year ago

@0xBugsy pls share reasoning for reduced severity. Thanks.

Was mistakenly gauging from other gas related issues that did not involve deposited assets for trading for example like #349 but I now see it has since then been updated to High.

c4-judge commented 1 year ago

trust1995 marked the issue as not selected for report

c4-judge commented 1 year ago

trust1995 marked the issue as duplicate of #607

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor acknowledged

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor confirmed

0xBugsy commented 1 year ago

We recognize the audit's findings on Anycall Gas Management. These will not be rectified due to the upcoming migration of this section to LayerZero.