Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/ulysses-omnichain/ArbitrumBranchBridgeAgent.sol#L156-L169

Vulnerability details

Impact

There is a re-entrancy attack potential in the function ArbitrumBranchBridgeAgent::_payExecutionGas. Storage remoteCallDepositedGas is deleted after the SafeTransferLib.safeTransferETH call, which allow an attacker to hijack the control flow and re-enter against this storage.

After analisys remoteCallDepositedGas for Arbitrum should be always 0 as described in _gasSwapIn (Gas already provided by Root Bridge Agent), so in theory this cannot bring any negative impact on that specific storage variable, but this might give an attacker room todo a deeper cross-contract re-entrancy attack as gas is not limited either on the safeTransferETH call.

It could be a normal re-enter or trying to impersonate the ArbitrumBranchBridgeAgent as there are many payable external function to re-enter as delegate (since if (gasRemaining > 0) is ensuring msg.value cannot be zero, so re-enter with delegation can only be done against payable function) .

Even if the impact is unknown, I think this still clasify as Medium and should be fixed. Re-entrancy attack are very common and sneaky and should not be left there, even if impact is unclear.

Proof of Concept

None as enought to undertsand the point I'm raising from code examination.

Tools Used

Code examination

Recommended Mitigation Steps

Firstly, the team should implement proper Check Effects Interactions pattern here to be safe which is properly implemented in BranchBridgeAgent but not in the Arbitrum version, so delete the storage before the transfer as shown in the code below.

function _payExecutionGas(address _recipient, uint256) internal override {
    //Get gas remaining
    uint256 gasRemaining = wrappedNativeToken.balanceOf(address(this));

    if (gasRemaining > 0) {
        //Unwrap Gas
        wrappedNativeToken.withdraw(gasRemaining);

        delete(remoteCallDepositedGas);

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

Secondly, in order to kill any re-entrancy as a whole, there should be a security like in BranchBridgeAgent to limit the gas being used for the transfer (by forcing a revert in case higher than TRANSFER_OVERHEAD).

Assessed type

Reentrancy

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

Re-entrancy attack with unknown potential impact #92