Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protocol earnings are permanently lost/locked #60

Closed
howlbot-integration bot opened this issue Sep 16, 2024 · 2 comments
Closed

Protocol earnings are permanently lost/locked #60

howlbot-integration bot opened this issue Sep 16, 2024 · 2 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-84 🤖_38_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

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

Vulnerability details

Impact

It will be impossible to withdraw the accrued protocol fees, so funds are permanently locked inside the contract.

Proof of Concept

The collect_protocol function is designed to transfer the FUSDC and pool token protocol fees to a recipient:

    #[allow(non_snake_case)]
    pub fn collect_protocol_7540_F_A_9_F(
        &mut self,
        pool: Address,
        amount_0: u128,
        amount_1: u128,
        recipient: Address,
    ) -> Result<(u128, u128), Revert> {
        assert_eq_or!(
            msg::sender(),
            self.seawater_admin.get(),
            Error::SeawaterAdminOnly
        );

        let (token_0, token_1) = self
            .pools
            .setter(pool)
            .collect_protocol(amount_0, amount_1)?;

->      erc20::transfer_to_addr(recipient, pool, U256::from(token_0))?;
->      erc20::transfer_to_addr(recipient, FUSDC_ADDR, U256::from(token_1))?;

        #[cfg(feature = "log-events")]
        evm::log(events::CollectProtocolFees {
            pool,
            to: recipient,
            amount0: token_0,
            amount1: token_1,
        });

        // transfer tokens
        Ok((token_0, token_1))
    }

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

The issue is that the token address and recipient are inverted in order, so the previous calls will always revert, as they use the recipient as the token address instead:

	pub fn transfer_to_addr(token: Address, recipient: Address, amount: U256) -> Result<(), Error> {
	    safe_transfer(token, recipient, amount)
	}

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

Tools Used

Manual Review

Recommended Mitigation Steps

Consider fixing the argument order:

    #[allow(non_snake_case)]
    pub fn collect_protocol_7540_F_A_9_F(
        &mut self,
        pool: Address,
        amount_0: u128,
        amount_1: u128,
        recipient: Address,
    ) -> Result<(u128, u128), Revert> {
        assert_eq_or!(
            msg::sender(),
            self.seawater_admin.get(),
            Error::SeawaterAdminOnly
        );

        let (token_0, token_1) = self
            .pools
            .setter(pool)
            .collect_protocol(amount_0, amount_1)?;

-       erc20::transfer_to_addr(recipient, pool, U256::from(token_0))?;
-       erc20::transfer_to_addr(recipient, FUSDC_ADDR, U256::from(token_1))?;

+       erc20::transfer_to_addr(pool, recipient, U256::from(token_0))?;
+       erc20::transfer_to_addr(FUSDC_ADDR, recipient, U256::from(token_1))?;

        #[cfg(feature = "log-events")]
        evm::log(events::CollectProtocolFees {
            pool,
            to: recipient,
            amount0: token_0,
            amount1: token_1,
        });

        // transfer tokens
        Ok((token_0, token_1))
    }

Assessed type

Token-Transfer

@howlbot-integration howlbot-integration bot added 3 (High Risk) Assets can be stolen/lost/compromised directly 🤖_38_group AI based duplicate group recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 16, 2024
howlbot-integration bot added a commit that referenced this issue Sep 16, 2024
@af-afk af-afk added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Sep 17, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked issue #84 as primary and marked this issue as a duplicate of 84

@c4-judge c4-judge added duplicate-84 satisfactory satisfies C4 submission criteria; eligible for awards and removed primary issue Highest quality submission among a set of duplicates labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-84 🤖_38_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

2 participants