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

Missing update current tick when we call set_sqrt_price_F_F_4_D_B_98_C() #63

Open
howlbot-integration bot opened this issue Sep 16, 2024 · 6 comments
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons 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/4528c9d2dbe1550d2660dac903a8246076044905/pkg/seawater/src/lib.rs#L1078-L1092

Vulnerability details

Impact

When we update the sqrt price via set_sqrt_price_F_F_4_D_B_98_C(), the pool's cur tick is not updated timely. This will cause that cur_tick is incorrect. Calculations based on cur_tick will be wrong.

Proof of Concept

In lib.rs, if one pool is misconfigured at the beginning of the pool's life, we can update the pool's sqrt price via function set_sqrt_price_F_F_4_D_B_98_C().
The problem is that the pool's cur tick is calculated based on the sqrt price. We only update the sqrt price, missing update the cur_tick.

For example:

  • Init this pool with one incorrect sqrt price, cur_tick will be calculated based on the incorrect sqrt price.
  • Then we update sqrt price via function set_sqrt_price_F_F_4_D_B_98_C.
    But the cur_tick is still incorrect.
    pub fn set_sqrt_price_F_F_4_D_B_98_C(
        &mut self,
        pool: Address,
        new_price: U256,
    ) -> Result<(), Revert> {
        assert_eq_or!(
            msg::sender(),
            self.seawater_admin.get(),
            Error::SeawaterAdminOnly
        );
        // @audit if we set the sqrt price incorrectly, and we want to set the price correctly, we should update the cur_tick together.
        self.pools.setter(pool).set_sqrt_price(new_price);

        Ok(())
    }

Tools Used

Manual

Recommended Mitigation Steps

Update the related cur_tick when we update the sqrt price.

Assessed type

Context

@howlbot-integration howlbot-integration bot added 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 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 acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons label Sep 17, 2024
@af-afk
Copy link

af-afk commented Sep 17, 2024

We won't fix this, since the admin is responsible for calling this path, and it should be aware of the risks/issues involved. It would be better to include another function that sets the current tick.

@alex-ppg
Copy link

The Warden outlines that the administrative function configuring the square root price of a particular pool will not update its current tick immediately, however, the submission fails to pinpoint the impact accurately other than a vague identification that the current tick would be incorrect.

Given the administrative nature of the function and the lack of adequate substantiation as to why the change would be harmful, I am inclined to consider this a QA caveat that should be documented for administrators who interact with the system.

@c4-judge c4-judge added downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg changed the severity to QA (Quality Assurance)

@c4-judge
Copy link
Contributor

alex-ppg marked the issue as grade-b

@c4-judge c4-judge added grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed grade-b labels Sep 23, 2024
@c4-judge
Copy link
Contributor

alex-ppg marked the issue as grade-c

@c4-judge c4-judge reopened this Oct 7, 2024
@c4-judge c4-judge removed grade-c unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Oct 7, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 7, 2024

alex-ppg marked the issue as grade-b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working downgraded by judge Judge downgraded the risk level of this issue grade-b primary issue Highest quality submission among a set of duplicates Q-11 QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants