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

Users can't remove liquidity while a pool is disabled #31

Open
c4-bot-4 opened this issue Sep 12, 2024 · 3 comments
Open

Users can't remove liquidity while a pool is disabled #31

c4-bot-4 opened this issue Sep 12, 2024 · 3 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 M-08 primary issue Highest quality submission among a set of duplicates 🤖_59_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report 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

@c4-bot-4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

There is a functionality for disabling a pool in case of unforeseen circumstances or if a problem occurs. While the pool is disabled the users should not be able to do certain actions but removing their liquidity should not be one of them.

When a problem occurs in the pool, users should be able to remove their liquidity, as it may be at risk and they may lose their money. The problem is there is a check that does not allow them to do it.

Proof of Concept

update_position_internal has a comment that states "Requires the pool to be enabled unless removing liquidity.", meaning that there should be a check for the pool status only when liquidity is added and the users should be allowed to remove their liquidity if the pool is not active.

But update_position_internal calls update_position that is used to add or remove liquidity from a position and the function has this check:

        assert_or!(self.enabled.get(), Error::PoolDisabled);

The check above prevents the function from being called when the pool is disabled, which is understandable in case users want to add liquidity to the pool when there is a problem.

However, users don't want their money to be at risk and they would want to remove their funds but they can't because of that check.

Tools Used

Manual Review

Recommended Mitigation Steps

If the passed delta is negative, it means the user is removing liquidity, so check the pool's status only when the delta is positive.

Assessed type

Access Control

@c4-bot-4 c4-bot-4 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 labels Sep 12, 2024
c4-bot-6 added a commit that referenced this issue Sep 12, 2024
@c4-bot-13 c4-bot-13 added the 🤖_59_group AI based duplicate group recommendation label Sep 13, 2024
@howlbot-integration howlbot-integration bot added primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels 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 18, 2024
@alex-ppg
Copy link

The submission and its duplicates have correctly identified that liquidity withdrawals are disallowed when the pool is paused despite what the code's documentation indicates.

I believe a medium-risk severity rating is appropriate given that important functionality of the protocol is inaccessible in an emergency scenario when it should be accessible.

@c4-judge
Copy link
Contributor

alex-ppg marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report 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 M-08 primary issue Highest quality submission among a set of duplicates 🤖_59_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report 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

6 participants