search

code-423n4 / 2022-01-livepeer-findings

0 stars 0 forks source link

`migrateETH` will not work #236

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

gzeon

Vulnerability details

Impact

migrateETH does not send the withdrawn ETH to L2 causing fund to stuck in the L1Migrator contract.

Proof of Concept

When migrateETH is called, it would withdraw all ETH from bridgeMinter, and then use sendTxToL2 create a L2 retryable ticket to call the L2Migrator with _l1CallValue set to msg.value and _l2CallValue set to the ETH balance withdrew.

https://github.com/livepeer/protocol/blob/20e7ebb86cdb4fe9285bf5fea02eb603e5d48805/contracts/token/BridgeMinter.sol#L90

    function withdrawETHToL1Migrator() external onlyL1Migrator returns (uint256) {
        uint256 balance = address(this).balance;

        // call() should be safe from re-entrancy here because the L1Migrator and l1MigratorAddr are trusted
        (bool ok, ) = l1MigratorAddr.call.value(address(this).balance)("");
        require(ok, "BridgeMinter#withdrawETHToL1Migrator: FAIL_CALL");

        return balance;
    }

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1Migrator.sol#L303

    function migrateETH(
        uint256 _maxGas,
        uint256 _gasPriceBid,
        uint256 _maxSubmissionCost
    ) external payable whenNotPaused {
        uint256 amount = IBridgeMinter(bridgeMinterAddr)
            .withdrawETHToL1Migrator();

        // Any ETH refunded to the L2 alias of this contract can be used for
        // other cross-chain txs sent by this contract.
        // The retryable ticket created will not be cancellable since this contract
        // currently does not support cross-chain txs to call ArbRetryableTx.cancel().
        // Regarding the comment below on this contract receiving refunds:
        // msg.sender also cannot be the address to receive refunds as beneficiary because otherwise
        // msg.sender could cancel the ticket before it is executed on L2 to receive the L2 call value.
        sendTxToL2(
            l2MigratorAddr,
            address(this), // L2 alias of this contract will receive refunds
            msg.value,
            amount,
            _maxSubmissionCost,
            _maxGas,
            _gasPriceBid,
            ""
        );
    }

The sendTxToL2 function create a L2 retryable ticket using _l1CallValue as call value. The ETH in _l2CallValue is not transfered to the inbox contract.

https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/gateway/L1ArbitrumMessenger.sol#L55

    function sendTxToL2(
        address target,
        address from,
        uint256 _l1CallValue,
        uint256 _l2CallValue,
        uint256 maxSubmissionCost,
        uint256 maxGas,
        uint256 gasPriceBid,
        bytes memory data
    ) internal returns (uint256) {
        uint256 seqNum = inbox.createRetryableTicket{value: _l1CallValue}(
            target,
            _l2CallValue,
            maxSubmissionCost,
            from,
            from,
            maxGas,
            gasPriceBid,
            data
        );
        emit TxToL2(from, target, seqNum, data);
        return seqNum;
    }

Since the _l2CallValue (i.e. the ETH withdrew from bridgeMinter) was never sent to the inbox, it would stuck in the L1 migrator contract and the L2 call would likely revert due to insufficient call value.

Recommended Mitigation Steps

The call value should be _l1CallValue + _l2CallValue

yondonfu commented 2 years ago

Duplicate of https://github.com/code-423n4/2022-01-livepeer-findings/issues/205