sherlock-audit / 2023-05-ecoprotocol-judging

1 stars 0 forks source link

branch_indigo - L2Rebase Vulnerable to L1 MEV Attack or Sequencer Front-Running Potentially Causing Excessive L2ECO Minting #156

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

branch_indigo

high

L2Rebase Vulnerable to L1 MEV Attack or Sequencer Front-Running Potentially Causing Excessive L2ECO Minting

Summary

ECO rebase on-chain execution is generally a two-step process. First, L1 bridge initiates rebase with L1 messenger broadcasting the message on mainnet. Second, L2 messenger relay message received from L1 messenger and invoke rebase on L2 bridge.

There are inherent risks in cross-chain communications. The first step is vulnerable to MEV attacks, and the second step could potentially be front run by the sequencer. Since ECO rebase is a central feature of the protocol and has a direct impact on ECO valuation and user balances, extra protection is needed to guard against disruption in token valuations and token transfers during rebasing.

Current implementation of both rebase on L2ECOBridge.sol and transferFrom on L2ECO.sol doesn't have any mechanisms to guard against risk as mentioned above. Specifically, during rebase, there is a window of temporary discrepancies in user token balances between L1 and L2 that could be exploited.

Vulnerability Detail

rebase directly changes a scaler(inflationMultiplier) that adjusts token balances. When inflationMultiplier increases, users balance decrease, creating deflationary pressure. When inflationMultiplier decreases, user balance increases, creating inflationary pressure. Based on the sponsor, the scaler value (inflationMultiplier) changes every two weeks and its value is the result of governance voting process as monetary control.

On mainnet, after new inflation multiplier is set,rebase on L1ECOBridge.sol can be called which creates the message to L2ECOBridge to invoke rebase on L2ECOBridge.sol. This transaction can potentially be delayed or front ran through MEV attacks. And MEV attacks on L1 can have an impact on the ordering of transactions on L2. In pre-bedrock optimism, transactions are ordered on the order in which they are received, so when exploiter delayed the broadcast sending a message to L2, it is very likely results in the delay of the corresponding transaction on L2. In optimism bedrock, even though a mempool is introduced but because the sequencer remains the sole entity to decide the transaction order, it's very likely that MEV attacks have the same although less direct effect on L2.

//L1ECOBridge.sol-rebase()
        bytes memory message = abi.encodeWithSelector(
            IL2ECOBridge.rebase.selector,
            inflationMultiplier
        );

        sendCrossDomainMessage(l2TokenBridge, _l2Gas, message);

On L2, rebase on L2ECOBridge.sol would only be invoked by L2Messenger, and it sets the new inflation multiplier both in its storage and on L2ECO token. This transaction can potentially be front ran by the sequencer. Under current Optimism, the sequencer is highly centralized with high centralization risk should it acts maliciously. Under optimism bedrock, there is a mempool and the order of transaction in a block is decided by the sequencer. When a sequencer is down or on the chance that it acts maliciously, it's possible that rebase from the L2 bridge could be delayed. In addition, there is also a near-future scenario where the sequencer becomes decentralized, then rebase is exposed to similar risks of MEV attack on L1 and be easily front ran by exploiters.

//L2ECOBridge.sol-rebase()
        inflationMultiplier = _inflationMultiplier;
        l2Eco.rebase(_inflationMultiplier);

Whenever rebase is delayed in being included in the block based on any scenarios above, the exploiter can deposit, finalize the deposit on L2 and traded L2ECO tokens with coins with more stable valuations (stable coins) to hedge against losing L2ECO token balances in the short run due to an inflation multiplier increase. This path is enabled because they are able to realize their token balances instantly after finalizeDeposit from L2ECOBridge.sol.

//L2ECOBridge.sol-finalizeDeposit()
        _amount = _amount / inflation multiplier;
        L2ECO(_l2Token).mint(_to, _amount);

They can instantly call transferFrom on L2ECO.sol to transfer their balance to an AMM for instance to trade for a different token. If they are able to front run L2rebase, then transferFrom under the hood would transfer their token balance based on the old inflation multiplier through _beforeTokenTransfer. When the new inflation multiplier is higher, this allows them to instantly trade with more ECO balances compared to after L2 rebase, gaining profits before a broader monetary controlled deflationary pressure takes hold.

//L2ECO.sol-_beforeTokenTransfer()
        amount = super._beforeTokenTransfer(from, to, amount);
        // overwrite for efficiency
        //gonsAmount is returned and emitted here
        amount = amount * linearInflationMultiplier;

See a test and results here as POC.

In the test, the exploiter managed to transfer out more L2ECO token balance to trade in comparison with Alice who deposits the same amount but transfers out her token after L2 rebase.

Impact

The above-mentioned attack creates disruption in ECO monetary control crucial to the protocol's health. When the inflation multiplier increases, the scaled balance decreases creating deflationary pressure. However, though MEV or Sequencer front-running, it's possible that exploiters are able to deposit large amount of ECO tokens to L2 and trade them out instantly before L2 rebase. This potentially causes a larget amount of L2ECO minting in a very short period of time, depending on the scale of the transfer, this could create acute inflationary pressure on L2 in contrary to the intention of the governance monetary control.

Although such trading could also take place on L1 without L2 depositing, it would be less profitable for exploiters, and also less damaging to ECO valuations due to no minting or burning involved. It's likely users would take advantage of L2 depositing to save on fees , or in the case for users who lose on the first opportunity to trade on L1, L2 would be a desirable path to realize such gains.

Code Snippet

see above. https://github.com/eco-association/op-eco/blob/39f205fb46eea3df770f0119a890ffdc1ac8f382/contracts/bridge/L2ECOBridge.sol#L162

Tool used

Manual Review

Recommendation

Possibly delay minting of L2ECO token in finalizeDeposit on L2ECOBridge.sol. So exploiters wouldn't be able to instantly trade on freshly deposited assets with an old inflation multiplier.

Duplicate of #52

sherlock-admin commented 1 year ago

Escalate for 10 USDC I think this is wrongly classified as a duplicate of #52 . Although both issues mentioned MEV during planned inflationMultiplier updates, (1) this issue varies in citing a different attack vector which is trading out L2 tokens through external L2 AMMs. This is different from transferring tokens within ECO bridging contracts as cited in issue 52. (2) this issue also claims different impacts from issue 52. As opposed to the loss of user ECO tokens, this issue claims the impact is an unintended acute inflationary pressure on L2 when inflationMultiplier increases, and that the inflationary pressure on L2 is contrary to the ECO monetary control direction which is creating a deflationary pressure. And this causes disruption in ECO monetary control.

In addition, I would like to defend this issue. Although within ECO briding contracts, the delay in inflationMultiplier update on L2 doesn't create a loss in ECO tokens balance between L1/L2 bridges as stated in sponsor comment on #52, the delay has a consequence when viewing it outside the bridging contract logics of balance calculation and creates an impact on the protocol.

A token asset price between L1 and L2 can be different due to variations in liquidity and trading activities on the specific chain. In addition, limitations or restrictions moving tokens between L1 and L2, creates price discrepancies as well. ECO token is especially sensitive to this difference because of its rebasing nature and its monetary control at the core of the protocol. This should be taken into account when viewing the effect of delaying in infaltionMultiplier update on L2.

When there is a window of delay in inflationMultiplier update, the stored unscaled balance of a user doesn't change but scaled user balance would be impacted which makes a difference when a user trading ECO out with other tokens to safe guard a pre-inflation or deflation token value. And this become financial incentive for MEV attacks to front run inflationMultiplier update. Specifically, when assets are bridged from L1 to L2, L2 minting would immediately available(after settlement) to token holders to realize any external trades. Because of minting on L2, during the window of delay, excessive inflation pressure is created on L2. The excessive minting together with increasing trading volume of L2 ECO within a short window, would create inflationary pressure in the opposite direction of ECO governance monetary control. And this one-time inflationary pressure, when compounding over time during multiple inflationMultiplier updates, might further drive the difference between ECO pricing between L1 and L2.

Thus, I believe it's very important to mitigate and minimize the inflationMultiplier delay during cross chain variable updates.

You've deleted an escalation for this issue.
JeffCX commented 1 year ago

Q: Please list any known issues/acceptable risks that should not result in a valid finding. AMM (or any external contract) arbitrage due to rebasing is not a valid finding.

we are in the process of extensively verifying the finding 52 though!

albertnbrown commented 1 year ago

This is correctly marked as a duplicate of #52 and it seems like what you're adding is just addressed by @JeffCX above reminding you that AMM and external trading of our token is not a valid finding. We have measures already in place to deal with this explained in #149