Lp's liquidity may be lost if re-org happens

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/lib.rs#L529-L543

Vulnerability details

Impact

LP holders will remove liquidity and then burn their liquidity position. If re-org happens, malicious users can manipulate the pool's price to cause user's removing liquidity revert. Then LP holders' position will be burned and liquidity will be locked in contract.

Proof of Concept

In lib.rs, users can burn their position. From this function's comment, Calling this function leaves any liquidity or fees left in the position inaccessible., it's users' responsibility to remove their liquidity before they burn their position.
The problem is that even if users follow this rule, remove liquidity at first, and then burn their position, users may still lose their funds.

Re-org is one common scenario in Arbitrum. When re-org happens, some transactions will be re-ordered and re-executed.
For example:

1. Alice removes her liquidity with one slippage control parameter.
2. Alice burns her position.
3. Re-org happens. Bob monitors the re-org and try to do some swap in pool to change pool's price and tick.
4. Alice removing liquidity transaction will be executed and reverted because Bob changes current pool's price and Alice's slippage condition will not be matched.
5. Alice's burning position can still be executed successfully.
6. Alice will lose her funds.

    pub fn burn_position_AE401070(&mut self, id: U256) -> Result<(), Revert> {
        let owner = msg::sender();
        // Get this NFT(position)'s owner to make sure that the caller owns this position.
        assert_eq_or!(
            self.position_owners.get(id),
            owner,
            Error::PositionOwnerOnly
        );

        self.remove_position(owner, id);

        #[cfg(feature = "log-events")]
        evm::log(events::BurnPosition { owner, id });

        Ok(())
    }

Tools Used

Manual

Recommended Mitigation Steps

Add some more check on burn_position_AE401070(), revert if there are some left liquidity.

Assessed type

Context

[af-afk]

We're going to resolve this by deleting the burn position feature.

[alex-ppg]

The Warden has identified a mechanism in the Seawater system that can become harmful if a re-organization occurs, leading to a total loss of the funds associated with the position.

I believe a medium risk severity rating is appropriate given the likelihood of a re-organization simultaneously occurring with the sequence of events outlined (i.e. different blocks that ended up being re-ordered executing the actions outlined) is low.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[0xSilvermist]

Arbitrum's docs states that a transaction can't be re-orged, so this should be invalid.

[blckhv]

Yeah, there are no re-orgs in Arbitrum - https://abarbatei.xyz/blockchain-reorgs-for-managers-and-auditors#heading-metrics-for-protocols-and-auditors

[alex-ppg]

Hey @0xSilvermist and @blckhv, thank you for your PJQA contributions. The documentation referenced states that if the L1 re-orgs, then the Arbitrum transactions will re-org as well. This is also referenced in the glossary itself.

The link referenced by @blckhv also does not say that "re-orgs are not possible", simply that they have not been observed yet. As such, the original ruling of the submission stands.

[blckhv]

Hey @alex-ppg,

I believe there is a difference between the re-org that can happen in Arbitrum vs Polygon, for example. Re-orgs in Arb are only if the sequencer is offline and then the only thing that will re-org are the transactions submitted the moment before the sequencer went offline before force-included ones.

We discussed the same topic in a recent contest and concluded that it can't happen: https://codehawks.cyfrin.io/c/2024-07-zaros/s/427

The docs that you provided summarises the same thing: https://docs.arbitrum.io/intro/glossary#reorg