code-423n4 / 2024-08-superposition-validation

0 stars 0 forks source link

Position transfer by nft manager can be front-runned by the owner who burnt the position #166

Closed c4-bot-10 closed 1 month ago

c4-bot-10 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L554

Vulnerability details

Impact

In the current implementation of the transfer_position_E_E_C7_A3_C_D() function, only nft manager is able to transfer user's position. But the problem is that the user can simply front-run any such transaction (transfer of position) by burning his nft.

Proof of Concept

Let's take a look at the transfer_position_E_E_C7_A3_C_D():

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/lib.rs#L554-569

  pub fn transfer_position_E_E_C7_A3_C_D(
        &mut self,
        id: U256,
        from: Address,
        to: Address,
    ) -> Result<(), Revert> {
        assert_eq_or!(msg::sender(), self.nft_manager.get(), Error::NftManagerOnly);

        self.remove_position(from, id);
        self.grant_position(to, id);

        #[cfg(feature = "log-events")]
        evm::log(events::TransferPosition { from, to, id });

        Ok(())
    }

So the function just removes position from the from address and grants position to the to address. However, any such transaction can be front-runned by just calling burn():

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

pub fn burn_position_AE401070(&mut self, id: U256) -> Result<(), Revert> {
        let owner = msg::sender();
        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(())
    }

So there is no any protection against front-running and there is no check if the position actually exists and is not already burnt.

Tools Used

Manual review.

Recommended Mitigation Steps

This issue can be mitigated by allowing only the owner to transfer the positions or disallow this functionality at all.

Assessed type

Other

rodiontr commented 4 weeks ago

I believe this issue should be transferred to the validation repo and judged as med for the following reasoning:

  1. This issue was confirmed by the sponsor in the following QA report: https://github.com/code-423n4/2024-08-superposition-findings/issues/168 (L-01)

  2. The issue demonstrates how NFT manager can be front-runned and lose gas fees as a result of a tx revert. So I suggest to upgrade it to medium severity