Livepeer contest - harleythedog's results

Handle

harleythedog

Vulnerability details

Impact

In L1Migrator.sol, the function migrateETH first withdraws eth from the BridgeMinter, and then intends to send all of this eth from L1 to L2. However, the parameters are incorrectly passed to the sendTxToL2 function, so none of this withdrawn eth will actually be transferred to the L2. The function will inadvertently keep all of this eth in the L1Migrator contract, where it will be frozen forever. This represents a large loss to the protocol, and the migration won't properly transfer the eth from L1 to L2, so this is a high severity issue.

Proof of Concept

See the code for migrateETH here, and see the code for sendTxToL2 here.

Now, notice that in migrateETH, the variable amount is the amount of eth that is supposed to be sent to the L2. This variable is passed as the fourth argument to sendTxToL2. However, looking at the implementation of sendTxToL2, the third argument is the amount of eth that is transferred to the L2, whereas the fourth argument is the call value that is associated with the transaction that executes the retryable ticket on the L2. According to the specification here, this means that the inbox will try to get amount eth from the L2 (where it should instead be getting it from the L1), and moreover the inbox isn't even going to send the amount eth to the L2 at all.

Tools Used

Manual inspection.

Recommended Mitigation Steps

After discussion with the sponsor on discord, it was determined that this bug is indeed valid and that the ideal fix would be to change the existing code:

sendTxToL2(
            l2MigratorAddr,
            address(this), // L2 alias of this contract will receive refunds
            msg.value,
            amount,
            _maxSubmissionCost,
            _maxGas,
            _gasPriceBid,
            ""
        );

to instead be:

sendTxToL2(
            l2MigratorAddr,
            address(this), // L2 alias of this contract will receive refunds
            msg.value + amount,
            amount,
            _maxSubmissionCost,
            _maxGas,
            _gasPriceBid,
            ""
        );

If you inspect the documentation here again closely, you can see that this works, since msg.value + amount eth will be transferred to the L2 first, and then amount of eth will be use as the call value in the retryable ticket. This mitigates the issue.

Handle

harleythedog

Vulnerability details

Impact

In L1Migrator.sol the function migrateLPT can be called by anyone. A malicious user can call migrateLPT with a small _maxSubmissionCost argument to intentionally make the retryable ticket creation fail. This will lock the LPT in the L1 escrow. Now, technically right now the admins would be able to retrieve these locked funds because the L1 escrow has the approve function. However, in a previous finding, I argued that this function gives the admins too much power so I recommended only allowing for approvals to the LPTGateway. So, in my opinion, this issue is a high severity issue as with the best codebase (that disallows admins rugging users), it is impossible to recover the locked LPT. At the very least, this griefing attack locks funds temporarily in the L1 escrow when they should be on L2.

Proof of Concept

Suppose a malicious user calls migrateLPT with _maxSubmissionCost == 0. According to the offchain labs documentation here and here, when you create a retryable ticket, if you underpay on the base submission cost, the ticket creation will fail even though the L1 transaction gets confirmed (the docs also note that this can be potentially be "very bad"). In the case of migrateLPT, this means that the LPT will have been transferred to the L1 escrow, but the retryable ticket will not be created so the corresponding L2 transaction will never be executed. This means that the LPT will be locked in the L1 escrow.

Tools Used

Manual inspection.

Recommended Mitigation Steps

In my opinion, only an admin should be able to call migrateLPT in order to ensure _maxSubmissionCost (and other param) values are set appropriately and retryable ticket creation goes smoothly. So I would recommend adding an "onlyAdmin" modifier to the function.

Handle

harleythedog

Vulnerability details

Impact

The L1Escrow contract has the function approve that is callable by the admin to approve an arbitrary spender with an arbitrary amount (so they can steal all of the escrow's holdings if they want). Even if the admin is well intended, the contract can still be called out which would degrade the reputation of the protocol (e.g. see here: https://twitter.com/RugDocIO/status/1411732108029181960). LPT is valuable on the Ethereum mainnet, so this rug vector should be mitigated. It would be best to restrict this function's power by only allowing approvals to other trusted protocol contracts (like L1LPTGateway, which I believe uses the escrow's approval).

NOTE: Even if the admin is under a timelock, this is still an issue, as users have to wait a whole week to withdraw from L2 -> L1 due to the dispute period.

Proof of Concept

See the approve function here: https://github.com/livepeer/arbitrum-lpt-bridge/blob/ebf68d11879c2798c5ec0735411b08d0bea4f287/contracts/L1/escrow/L1Escrow.sol#L21

Tools Used

Inspection.

Recommended Mitigation Steps

Restrict the power of this approve function so that the admin isn't able to steal funds. This can be accomplished by only allowing approvals to other protocol functions (instead of arbitrary approvals).