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

24 stars 13 forks source link

Attacker can `retrySettlement` by virtual account to steal accumulated fee in root bridge agent #349

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/54a45beb1428d85999da3f721f923cbf36ee3d35/src/ulysses-omnichain/RootBridgeAgent.sol#L759

Vulnerability details

Impact

Excess gas fee is accumulated within root bridge agent as WETH. Attacker can retrySettlement by virtual account to steal the accumulated fee.

Proof of Concept

function retrySettlement(uint32 _settlementNonce, uint128 _remoteExecutionGas) external payable {
    //Update User Gas available.
    if (initialGas == 0) {
        userFeeInfo.depositedGas = uint128(msg.value);
        userFeeInfo.gasToBridgeOut = _remoteExecutionGas;
    }
    //Clear Settlement with updated gas.
    _retrySettlement(_settlementNonce);
}

function _retrySettlement(uint32 _settlementNonce) internal returns (bool) {
    Settlement memory settlement = getSettlement[_settlementNonce];
    if (settlement.owner == address(0)) return false;

    bytes memory newGas = abi.encodePacked(_manageGasOut(settlement.toChain));
    for (uint256 i = 0; i < newGas.length;) {
        settlement.callData[settlement.callData.length - 16 + i] = newGas[i];
        unchecked {
            ++i;
        }
    }
    ......
    _performCall(settlement.callData, settlement.toChain);

    return true;
}

function _manageGasOut(uint24 _toChain) internal returns (uint128) {
    uint256 amountOut;
    address gasToken;
    uint256 _initialGas = initialGas;
    ......
    if (_initialGas > 0) {
        if (userFeeInfo.gasToBridgeOut <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGasForFees();
        (amountOut, gasToken) = _gasSwapOut(userFeeInfo.gasToBridgeOut, _toChain);
    } else {
        if (msg.value <= MIN_FALLBACK_RESERVE * tx.gasprice) revert InsufficientGasForFees();
        wrappedNativeToken.deposit{value: msg.value}();
        (amountOut, gasToken) = _gasSwapOut(msg.value, _toChain);
    }

    IPort(localPortAddress).burn(address(this), gasToken, amountOut, _toChain);
    return amountOut.toUint128();
}

Steps:

  1. Prepare some failed settlements by callOutSigned with zero _remoteExecutionGas and non-arb toChain.
  2. callOutSigned with nonzero _remoteExecutionGas.
  3. Virtual account calls retrySettlement with a failed nonce. initialGas will be greater than zero and userFeeInfo.gasToBridgeOut amount of WETH will be swapped into remote chain gas token.
  4. Receive remaining gas token when _payExecutionGas at branch chain.
  5. Virtual account repeats 3 until accumulated WETH is drained.

Note: It's better to attack with a larger _remoteExecutionGas so that less gas will be wasted by multiple executions.

function _gasSwapIn(bytes memory gasData) internal virtual returns (uint256 gasAmount) {
    //Cast to uint256
    gasAmount = uint256(uint128(bytes16(gasData)));
    //Move Gas hTokens from Branch to Root / Mint Sufficient hTokens to match new port deposit
    IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), gasAmount);
}

function _payExecutionGas(address _recipient, uint256 _initialGas) internal virtual {
    uint256 gasRemaining = wrappedNativeToken.balanceOf(address(this));
    wrappedNativeToken.withdraw(gasRemaining);
    ......
    _replenishGas(minExecCost);
    SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);
    ......
}

Tools Used

Manual

Recommended Mitigation Steps

  1. Store availableGas at the beginning of anyExecute:
    uint256 availableGas = userFeeInfo.depositedGas - userFeeInfo.gasToBridgeOut;
  2. Delete userFeeInfo.gasToBridgeOut after _manageGasOut.
  3. Use availableGas as input for _payExecutionGas.

Assessed type

Reentrancy

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-judge commented 1 year ago

trust1995 changed the severity to 2 (Med Risk)

c4-sponsor commented 1 year ago

0xBugsy marked the issue as sponsor confirmed

c4-judge commented 1 year ago

trust1995 changed the severity to 3 (High Risk)

c4-judge commented 1 year ago

trust1995 marked the issue as selected for report

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 #645