Attacker can "retrieveDeposit" then "redeemDeposit" with a future nonce to freeze the assets of subsequent depositers

[code423n4]

Lines of code

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

Vulnerability details

Impact

Attacker can call retrieveDeposit with a future deposit nonce and anyFallback will be triggered to set the deposit status to Failed. Then, the attacker calls redeemDeposit to delete getDeposit[_depositNonce]. When a unlucky depositer makes a deposit with that nonce, everything will go as usual on branch chain, except that executionHistory[fromChainId][nonce] has already been marked as true on root chain. As a result, the root chain transaction will forcerevert() and the depositer has no way to get their assets back.

Proof of Concept

function retrieveDeposit(uint32 _depositNonce) external payable lock requiresFallbackGas {
    //Encode Data for cross-chain call.
    bytes memory packedData = abi.encodePacked(bytes1(0x08), _depositNonce, msg.value.toUint128(), uint128(0));

    //Update State and Perform Call
    _sendRetrieveOrRetry(packedData);
}

function anyExecute(bytes calldata data)
    external
    virtual
    requiresExecutor
    returns (bool success, bytes memory result)
{
    ......
        /// DEPOSIT FLAG: 8 (retrieveDeposit)
    } else if (flag == 0x08) {
        //Get nonce
        uint32 nonce = uint32(bytes4(data[1:5]));

        //Check if tx has already been executed
        if (!executionHistory[fromChainId][uint32(bytes4(data[1:5]))]) {
            //Toggle Nonce as executed
            executionHistory[fromChainId][nonce] = true;

            //Retry failed fallback
            (success, result) = (false, "");
        } else {
            _forceRevert();
            //Return true to avoid triggering anyFallback in case of `_forceRevert()` failure
            return (true, "already executed tx");
        }
    }
    ......
}

function _redeemDeposit(uint32 _depositNonce) internal {
    Deposit storage deposit = getDeposit[_depositNonce];

    // will be skipped since length is zero
    for (uint256 i = 0; i < deposit.hTokens.length;) {
        _clearToken(deposit.owner, deposit.hTokens[i], deposit.tokens[i], deposit.amounts[i], deposit.deposits[i]);

        unchecked {
            ++i;
        }
    }

    //Delete Failed Deposit Token Info
    delete getDeposit[_depositNonce];
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in retrieveDeposit:

require(getDeposit[_depositNonce].owner == msg.sender);

Assessed type

Invalid Validation

[c4-judge]

trust1995 marked the issue as unsatisfactory:
Insufficient proof

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as duplicate of #688