search

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

0 stars 0 forks source link

[WP-H3] `L1Migrator.sol#migrateETH()` Improper implementation of `L1Migrator` causing `migrateETH()` always reverts, can lead to ETH in `BridgeMinter` getting stuck in the contract #198

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

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

uint256 amount = IBridgeMinter(bridgeMinterAddr)
            .withdrawETHToL1Migrator();

L1Migrator.sol#migrateETH() will call IBridgeMinter(bridgeMinterAddr).withdrawETHToL1Migrator() to withdraw ETH from BridgeMinter.

However, the current implementation of L1Migrator is unable to receive ETH.

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

(bool ok, ) = l1MigratorAddr.call.value(address(this).balance)("");

A contract receiving Ether must have at least one of the functions below:

receive() is called if msg.data is empty, otherwise fallback() is called.

Because L1Migrator implement neither receive() or fallback(), the call at L94 will always revert.

Impact

All the ETH held by the BridgeMinter can get stuck in the contract.

Recommandation

Add receive() external payable {} in L1Migrator.

yondonfu commented 2 years ago

Severity: 2 (Med)

We'll fix this, but noting that the funds are recoverable because the BridgeMinter can set a new L1Migrator that does have the receive() function which is why the suggested severity is 2 (Med).

yondonfu commented 2 years ago

Fixed in https://github.com/livepeer/arbitrum-lpt-bridge/pull/50

0xleastwood commented 2 years ago

Agree with sponsor, these funds are recoverable. However, the warden has identified a DOS attack, which is a valid medium severity issue.