Incorrect accounting logic for fallback gas will lead to insolvency

[code423n4]

Lines of code

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

Vulnerability details

Impact

Incorrect accounting logic for fallback gas will lead to insolvency.

Proof of Concept

// on root chain
function _payExecutionGas(uint128 _depositedGas, uint128 _gasToBridgeOut, uint256 _initialGas, uint24 _fromChain) internal {
    ......
    uint256 availableGas = _depositedGas - _gasToBridgeOut;
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasleft());

    if (minExecCost > availableGas) {
        _forceRevert();
        return;
    }

    _replenishGas(minExecCost);

    //Account for excess gas
    accumulatedFees += availableGas - minExecCost;
}

// on branch chain
function _payFallbackGas(uint32 _depositNonce, uint256 _initialGas) internal virtual {
    ......
    IPort(localPortAddress).withdraw(address(this), address(wrappedNativeToken), minExecCost);
    wrappedNativeToken.withdraw(minExecCost);
    _replenishGas(minExecCost);
}

As above, when paying execution gas on the root chain, the excessive gas is added to accumulatedFees. So theoretically all deposited gas is used up and no gas has been reserved for anyFallback on the branch chain. The withdrawl in _payFallbackGas on branch chain will cause insolvency.

// on branch chain
function _payExecutionGas(address _recipient, uint256 _initialGas) internal virtual {
    ......
    uint256 gasLeft = gasleft();
    uint256 minExecCost = tx.gasprice * (MIN_EXECUTION_OVERHEAD + _initialGas - gasLeft);

    if (minExecCost > gasRemaining) {
        _forceRevert();
        return;
    }

    _replenishGas(minExecCost);

    //Transfer gas remaining to recipient
    SafeTransferLib.safeTransferETH(_recipient, gasRemaining - minExecCost);
    ......
    }
}

// on root chain
function _payFallbackGas(uint32 _settlementNonce, uint256 _initialGas) internal virtual {
    uint256 gasLeft = gasleft();
    uint256 minExecCost = tx.gasprice * (MIN_FALLBACK_RESERVE + _initialGas - gasLeft);

    if (minExecCost > getSettlement[_settlementNonce].gasToBridgeOut) {
        _forceRevert();
        return;
    }

    getSettlement[_settlementNonce].gasToBridgeOut -= minExecCost.toUint128();
}

As above, when paying execution gas on the branch chain, the excessive gas has be sent to the recipent. So therotically all deposited gas is used up and no gas has been reserved for anyFallback on the root chain. _payFallbackGas does not _replenishGas, which will cause insolvency of gas budget in AnycallConfig.

Tools Used

Manual

Recommended Mitigation Steps

Deduct fallback gas from deposited gas.

Assessed type

Context

[c4-judge]

trust1995 marked the issue as duplicate of #385

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-sponsor]

0xBugsy marked the issue as sponsor confirmed

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 marked the issue as selected for report

[c4-sponsor]

0xBugsy marked the issue as sponsor acknowledged

[c4-sponsor]

0xBugsy marked the issue as sponsor confirmed

[0xBugsy]

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.

[c4-judge]

trust1995 marked the issue as not selected for report

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 marked the issue as duplicate of #786

[xuwinnie]

Hey, I believe this is not a dup of #786. This issue is mainly about accounting logic. I have described two scenes

1. execute on root & fallback on branch: insolvency of port's weth balance.
2. execute on branch & fallback on root: insolvency of budget.

Even though fix from #786 is applied, the accounting logic is still incorrect, If port's balance is reduced, it comes to scene 1: insolvency of port's balance.

[xuwinnie]

And this issue will cause insolvency of h-weth, so I think it reaches high.

[0xBugsy]

As above, when paying execution gas on the root chain, the excessive gas is added to accumulatedFees. So theoretically all deposited gas is used up and no gas has been reserved for anyFallback on the branch chain. The withdrawl in _payFallbackGas on branch chain will cause insolvency.

1. This isn't accurate, fallback gas for a call from Root -> Branch is enforced and allocated in _manageGasOut not _payExecutionGas, so the proposed fix will not lead to hToken insolvency on Root (although the proposed fix should have the added detail that the balance should be obtained from bridgeToRoot and not a withdrawal and this can only be done once per failed deposit state meaning it would need to be set to true and FALLBACK_RESERVE replenished to be deducted again)

As above, when paying execution gas on the branch chain, the excessive gas has be sent to the recipent. So therotically all deposited gas is used up and no gas has been reserved for anyFallback on the root chain. _payFallbackGas does not _replenishGas, which will cause insolvency of gas budget in AnycallConfig.

2. This is also invalid since MIN_FALLBACK_RESERVE is enforced keeping deposited gas in Branch Port and gas is replenished upon _payFallbackGas withdrawing from Port in an appropriate manner

I believe this was marked as a duplicate owing to the fact that in 1. you described a situation in #786 where a error exists and proposed the same appropriate fix

[xuwinnie]

Thanks for explaining @0xBugsy. To make my point clearer, I'll give an example. Suppose a user retrieveDeposit and deposited 20 unit gas. depositedGas is 20 and gasToBridgeOut(remoteExecutionGas) is 0. On root chain the whole process does not involve _manageGasOut. In _payExecutionGas, suppose 12 unit is replenished, then 8 unit is added to accumulatedFees. On branch chain, fallback costs 14 gas, then 14 unit is withdrawn from port and replenished. Overall, 20 in 34 out. As you mentioned, I believe _manageGasOut should be used to manage fallback gas, but it seems to be only managing remote execution gas. I'm not sure I've understood everything correctly, if I misunderstood something please tell me.

[0xBugsy]

Hey, I believe your are not considering the fact that Fallback Gas is reserved every time a remote call is initiated, so if in your scenario you are calling retrieveDeposit this means that deposit already has fallback gas reserved in the origin branch and we are also sure that fallback is yet to be triggered so this balance has not been double spent. This is enforced directly in the callout functions in branches whereas in the Root this is enforced in the _manageGasOut where gas minimum is checked and assets are converted to destination chain gas.

Hope this made it clearer!

[0xBugsy]

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.

[xuwinnie]

Hey @trust1995 @0xBugsy sorry for delaying the judging process but I still need to add something. "that deposit already has fallback gas reserved in the origin branch and we are also sure that fallback is yet to be triggered so this balance has not been double spent." This is not true. The balance is double spent. Let's suppose the user deposited this gas in an tx on branch. On the root chain, although tx fails and anyExecute returns false, gas is still charged (since it is not forceReverted). So double spending occurs(on root anyExecute and branch anyFallback)!

[0xBugsy]

I believe there may have been some language barrier in our communication but what I now believe has happen is:

1. you disclosed everything that was covered in detail in #786
2. added the fact that opposed to what #786 claims porting the Branch functioning is not enough since once initiating a cross-chain call we should always deduct the chain's FALLBACK_RESERVE from the deposited gas (in root deduct branch fallback reserve gas units and in branch reverse ) which would mean the solution put forward in #786 is not 100% accurate complete .

By the way this was not at all made obvious in the issue took some reading between the lines but happy we go to some understanding and obviously do correct me if my interpretation of what was said is incorrect in any way.

[xuwinnie]

@0xBugsy Yeah, this is what I want to say! I'm sorry if my previous expression is not clear enough!

[xuwinnie]

Hi @trust1995, to conclude, the core issue I described here is double spending of deposited gas which will lead to insolvency of port's weth. I believe none of 786 or its dups has mentioned it. Thanks for your attention!

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as selected for report

[trust1995]

Upon further inspection, warden has uncovered a different root cause than previously dupped submissions. The risks associated are deemed of Medium severity.

[xuwinnie]

Oh, sorry for coming again @trust1995. Let me show how this can reach high. Actually attacker can cause a large loss of weth balance of branch port by exploiting this issue within a single tx. In performCallOut at branch chain, attacker can set bytes calldata _params with a huge length. In anyFallback at branch chain, there is a emit LogCalloutFail(flag, data, rootChainId);. They can serve as gas bomb. So the actual loss could be high. I'll go sleep now! If there is anything unclear I'll add tomorrow.

[trust1995]

@xuwinnie In C4 we judge based on the merits of the submission during contest time. QA period is meant to clarify validity rather than further developing a certain finding. This decision is now final.