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

Unset fee_protocol forces protocol fee loss during swaps #14

Closed
c4-bot-7 opened this issue Sep 8, 2024 · 2 comments
Closed

Unset fee_protocol forces protocol fee loss during swaps #14

c4-bot-7 opened this issue Sep 8, 2024 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-8 edited-by-warden 🤖_03_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@c4-bot-7
Copy link
Contributor

c4-bot-7 commented Sep 8, 2024

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L31
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L345-L346
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L356
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L449-L453
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L514
https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/pool.rs#L522

Vulnerability details

Impact

Loss of protocol fees during swaps as the fee_protocol variable wasn't set anywhere during the creation of pools and there is no setter function to set it once the pool is live. Hence, protocol fee accrual will be lost.

Proof of Concept

Consider this scenario:

  1. Pool is created (WETH/fUSDC)
  2. Users deposit liquidity into the pool
  3. Swaps begin (in every swap, the protocol is supposed to earn its own fees that go to the protocol and global pool fees for both tokens that go to liquidity providers in the pool)
  4. Protocol fees even out to zero hence, there are no protocol fees to collect when collect_protocol_7540_F_A_9_F function is called
let fee_protocol = match zero_for_one { // @note unset fee_protocol so it's 0 by default
            true => self.fee_protocol.get().sys() % 16, // @audit zeroes out
            false => self.fee_protocol.get().sys() >> 4, // @audit zeroes out
        };

This code block is skipped because fee_protocol zeroed out

// set fees
// @audit skipped
            if fee_protocol > 0 {
                let delta = step_fee_amount.wrapping_div(U256::from(fee_protocol));
                step_fee_amount -= delta;
                state.protocol_fee += u128::try_from(delta).or(Err(Error::FeeTooHigh))?;
            }
// update fees
        if fee != 0 {
            match zero_for_one {
                true => {
                    self.fee_growth_global_0.set(state.fee_growth_global);
                    if state.protocol_fee > 0 {
                        let new_protocol_fee =
                            self.protocol_fee_0.get() + U128::lib(&state.protocol_fee);
@>                        self.protocol_fee_0.set(new_protocol_fee); // 0
                    }
                }
                false => {
                    self.fee_growth_global_1.set(state.fee_growth_global);
                    if state.protocol_fee > 0 {
                        let new_protocol_fee =
                            self.protocol_fee_1.get() + U128::lib(&state.protocol_fee);
@>                        self.protocol_fee_1.set(new_protocol_fee); // 0
                    }
                }
            }
        }

Tools Used

Manual review

Recommended Mitigation Steps

Set fee_protocol when the pool is created or have a function for setting it.

Assessed type

Math

@c4-bot-7 c4-bot-7 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Sep 8, 2024
c4-bot-3 added a commit that referenced this issue Sep 8, 2024
@c4-bot-11 c4-bot-11 added the 🤖_03_group AI based duplicate group recommendation label Sep 13, 2024
@howlbot-integration howlbot-integration bot added sufficient quality report This report is of sufficient quality duplicate-8 labels Sep 16, 2024
@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg changed the severity to 2 (Med Risk)

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label 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
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue duplicate-8 edited-by-warden 🤖_03_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants