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

iamnmt - In SuperPool, an attacker can move assets to a specific base pool #487

Closed
sherlock-admin3 opened this issue Aug 24, 2024 · 6 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed

Comments

@sherlock-admin3
Copy link

sherlock-admin3 commented Aug 24, 2024

iamnmt

Medium

In SuperPool, an attacker can move assets to a specific base pool

Summary

In SuperPool, the design supply to pools and withdraw from pools in queue will allow an attacker to move assets to a specific base pool.

Root Cause

The design supply to pools and withdraw from pools in queue

https://github.com/sentimentxyz/protocol-v2/blob/04bf15565165396608cc0aedacf05897235518fd/src/SuperPool.sol#L524-L543
https://github.com/sentimentxyz/protocol-v2/blob/04bf15565165396608cc0aedacf05897235518fd/src/SuperPool.sol#L548-L580

Internal pre-conditions

Let's call a base pool that an attacker want to move assets is X.

  1. Every base pool before X in depositQueue have SuperPool#poolCapFor[poolId] != type(uint256).max or Pool#poolDataFor[poolId].poolCap != type(uint256).max. The meaning of this pre-condition is there is way to deposit to X.
  2. There is exist a base pool before X in withdrawQueue that the SuperPool has assets in.

External pre-conditions

No response

Attack Path

There is a SuperPool that has:

  • depositQueue = [A, B, X]
  • SuperPool#poolCapFor[A] = 100, Pool#poolDataFor[B].poolCap = 100
  • Pool#getTotalAssets(A) = 50, Pool#getTotalAssets(B) = 50, Pool#getTotalAssets(X) = 0
  • Pool#getAssetsOf(A, address(SuperPool)) = 50, Pool#getAssetsOf(B, address(SuperPool)) = 50, Pool#getAssetsOf(X, address(SuperPool)) = 0
  • withdrawQueue = [A, B, X]

This SuperPool is satisfied the internal pre-conditions. The base pool A represents for base pools that have SuperPool#poolCapFor[poolId] != type(uint256).max. The base pool B represents for base pools that have Pool#poolDataFor[poolId].poolCap != type(uint256).max. The goal of this attack is to move assets from A, B to X.

An attacker performs the attack in one transaction:

  1. Call to SuperPool#deposit(50, attacker). New state:
    • Pool#getTotalAssets(A) = 100
    • Pool#getAssetsOf(A, address(SuperPool)) = 100
  2. Call to Pool#deposit(B, 50, attacker). New state:
    • Pool#getTotalAssets(B) = 100
  3. Call to SuperPool#deposit(100, attacker). The SuperPool will deposit to X because SuperPool#poolCapFor[A], Pool#poolDataFor[B].poolCap are reached. New state:
    • Pool#getTotalAssets(X) = 100
    • Pool#getAssetsOf(X, address(SuperPool)) = 100
  4. Call to SuperPool#withdraw(100, attacker, attacker). New state:
    • Pool#getTotalAssets(A) = 0, Pool#getTotalAssets(B) = 0, Pool#getTotalAssets(X) = 100
    • Pool#getAssetsOf(A, address(SuperPool)) = 0, Pool#getAssetsOf(B, address(SuperPool)) = 0, Pool#getAssetsOf(X, address(SuperPool)) = 100
  5. Call to Pool#withdraw(B, 50, attacker). The attacker retrieves the funds deposited in step 2.

The attacker moved all assets to X. By doing this attack in one transaction, the attacker can flash-loan 150 tokens at the start of the attack for step 1 and 2, and then returns 150 tokens back at the end of the attack. Note that, the attacker does not hold any shares of SuperPool or Pool at the end of the attack. Meaning the cost this attack is only gas fee and flash-loan fee.

Impact

By moving assets to a specific base pool, an attacker can cause the following larger impacts:

  • Front-running PositionManager#liquidateBadDebt with this attack to cause loss of funds for the SuperPool. When the protocol calls PositionManager#liquidateBadDebt, a base pool that has its bad debt being liquidated will suffer a loss. So, the attacker will move assets from other pools to the pools that has its bad debt being liquidated, which will cause loss of funds to the SuperPool.
  • Use liquidity from other pools for withdrawing in the attacker's desired pool. Users can not call to Pool#withdraw when maxWithdrawAssets is not enough. In case of, the pool that the attacker want to withdraw from does not have enough liquidity, the attacker can perform this attack to move assets from other pools to their desired pool.
  • Move assets to a low performance pool to cause loss of yield for the SuperPool.

PoC

No response

Mitigation

Add a two-step SuperPool#deposit/mint. First the users stage their deposit/mint. After a short timelock (E.g: 10 seconds), the users can finalize their deposit/mint. This will prevent the attack that uses flash-loan, but if the attacker has enough liquidity, then this attack still can happen.

Duplicate of #564

@github-actions github-actions bot added the Medium A Medium severity issue. label Sep 5, 2024
@sherlock-admin4 sherlock-admin4 changed the title Overt Wintergreen Rabbit - In SuperPool, an attacker can move assets to a specific base pool iamnmt - In SuperPool, an attacker can move assets to a specific base pool Sep 15, 2024
@sherlock-admin4 sherlock-admin4 added the Reward A payout will be made for this issue label Sep 15, 2024
@S3v3ru5
Copy link

S3v3ru5 commented Sep 16, 2024

#564 is a duplicate of this.

Both exploits have same pre-conditions.

Qttacker causing reallocation of amounts in base pools by depositing and immediately withdrawing certain amount. Because of the difference in deposit queue and withdraw queue, deposit might happen into one pool A and withdraw can come different pool B. As a result, assets are reallocated from pool B to pool A.

The #564 lists the impact as:
The reallocation causes the base pool which is about incur bad debt to have more deposits and hence causing more losses to the SuperPool users when rebalanceBadDebt is called.

@0x3b33
Copy link

0x3b33 commented Sep 16, 2024

Escalate

Per the above comment. In short, these 2 issues have the same root cause.

@sherlock-admin3
Copy link
Author

Escalate

Per the above comment. In short, these 2 issues have the same root cause.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

@sherlock-admin4 sherlock-admin4 added the Escalated This issue contains a pending escalation label Sep 16, 2024
@sherlock-admin3 sherlock-admin3 added Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed labels Sep 17, 2024
@cvetanovv
Copy link
Collaborator

I agree it is a duplicate of #564.

Planning to accept the escalation and duplicate this issue with #564.

@WangSecurity
Copy link

Result:
Medium
Duplicate of #564

@sherlock-admin2 sherlock-admin2 removed the Escalated This issue contains a pending escalation label Sep 26, 2024
@sherlock-admin3 sherlock-admin3 added the Escalation Resolved This issue's escalations have been approved/rejected label Sep 26, 2024
@sherlock-admin4
Copy link
Contributor

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin3 sherlock-admin3 added the Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Escalation Resolved This issue's escalations have been approved/rejected Medium A Medium severity issue. Reward A payout will be made for this issue Sponsor Disputed The sponsor disputed this issue's validity Won't Fix The sponsor confirmed this issue will not be fixed
Projects
None yet
Development

No branches or pull requests

7 participants