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

hash - Attacker can inflict losses to other Superpool user's during a bad debt liquidation depending on the deposit/withdraw queue order #564

Open
sherlock-admin2 opened this issue Aug 24, 2024 · 10 comments
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A Medium severity issue. Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin2
Copy link
Contributor

sherlock-admin2 commented Aug 24, 2024

hash

Medium

Attacker can inflict losses to other Superpool user's during a bad debt liquidation depending on the deposit/withdraw queue order

Summary

Attacker can inflict losses to other Superpool user's during a bad debt liquidation depending on the deposit/withdraw queue order

Vulnerability Detail

On bad debt liquidation the underlying BasePool depositors eats losses

    function rebalanceBadDebt(uint256 poolId, address position) external {
        
        .....

        pool.totalDepositAssets = (totalDepositAssets > borrowAssets) ? totalDepositAssets - borrowAssets : 0;

In a superpool this allows an attacker to inflict more losses to others depending on the deposit/withdraw pool order without suffering any losses for himself if he can deposit more assets in the to be affected pool and withdraw from another pool

    function reorderDepositQueue(uint256[] calldata indexes) external onlyOwner {
        if (indexes.length != depositQueue.length) revert SuperPool_QueueLengthMismatch(address(this));
        depositQueue = _reorderQueue(depositQueue, indexes);
    }


    /// @notice Reorders the withdraw queue, based in withdraw priority
    /// @param indexes The new withdrawQueue, in order of priority
    function reorderWithdrawQueue(uint256[] calldata indexes) external onlyOwner {
        if (indexes.length != withdrawQueue.length) revert SuperPool_QueueLengthMismatch(address(this));
        withdrawQueue = _reorderQueue(withdrawQueue, indexes);
    }

Eg:
poolA = 100 value, 100shares
poolB = 100 value, 100shares
superPool deposit order [poolA,poolB]
superPool withdraw order [poolB,poolA]
superPool balance = 100 value, all deposited in poolB
bad debt liqudiation of 100 for poolA is about to happen
attacker deposits 100 value in superpool and withdraws 100
attacker suffers no loss
now superPool has entire balance in poolA
poolA = 200value , 200 shares
after bad debt liquidation, poolA = 100 value,200shares
this loss is beared by the other superpool depositors

Impact

Attacker can inflict losses to other superpool depositors

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/SuperPool.sol#L345-L355

Tool used

Manual Review

Recommendation

Monitor for bad debt and manage the bad debt pool

@github-actions github-actions bot added the Medium A Medium severity issue. label Sep 5, 2024
@sherlock-admin4 sherlock-admin4 changed the title Flat Tawny Haddock - Attacker can inflict losses to other Superpool user's during a bad debt liquidation depending on the deposit/withdraw queue order hash - Attacker can inflict losses to other Superpool user's during a bad debt liquidation depending on the deposit/withdraw queue order 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

This is a duplicate of #487

@Kalogerone
Copy link

Escalate as per above comment

@sherlock-admin3
Copy link

Escalate as per above comment

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
@0xdeth
Copy link

0xdeth commented Sep 17, 2024

Escalating this to make this a low.

All the following have to take place in order for this attack to be pulled off.

  • Very low likelihood to have bad debt. Liquidating bad debt is a last resort and liquidators should have liquidated the position long before the bad debt was hit.
  • Pool owner and reallocators are responsible to handle riskier pools, by changing deposit/withdraw orders, blocking deposits on riskier pools (setting poolCap = 0) or entirely removing riskier pools.
  • Having such pool order is not guaranteed and depositors trust owner and allocators to have the riskiest pool in the beginning of the withdraw queue, which makes the exploit path impossible in such cases.

We aren't arguing the issue is invalid, but the likelihood of all the above pieces falling in place are very low, thus we believe this is a Low severity issue.

@elhajin
Copy link

elhajin commented Sep 17, 2024

i think this is invalid , (Regardless of the very low likelihood)
let's analyze the example in both cases with the attack and normal case :

Scenario 1: No Attacker

Initial state:

  • PoolA: 100 Value, 100 Shares
  • PoolB: 100 Value, 100 Shares
  • SuperPool: 100 Shares in PoolA, 100 Shares in PoolB , superPoolShares is 200
  1. Bad debt of 100 Value slashed from PoolA:

    • PoolA: 0 Value, 100 Shares
    • PoolB: 100 Value, 100 Shares
    • SuperPool: 100 Shares in PoolA (worth 0 Value), 100 Shares in PoolB (worth 100 Value)
  2. Final state:

    • SuperPool share value = (0 + 100) / 200 = 0.5 Value per share

Scenario 2: With Attacker

Initial state:

  • PoolA: 100 Value, 100 Shares
  • PoolB: 100 Value, 100 Shares
  • SuperPool: 100 Shares in PoolA, 100 Shares in PoolB , superPoolShares is 200
  1. Attacker deposits 100 Value to PoolA through SuperPool:

    • PoolA: 200 Value, 200 Shares
    • PoolB: 100 Value, 100 Shares
    • SuperPool: 200 Shares in PoolA (worth 200 Value), 100 Shares in PoolB (worth 100 Value) , superPoolShares is 300
  2. Attacker withdraws from PoolB:

    • PoolA: 200 Value, 200 Shares
    • PoolB: 0 Value, 0 Shares
    • SuperPool: 200 Shares in PoolA (worth 200 Value), 0 Shares in PoolB , superPoolShares is 200
  3. Bad debt of 100 Value slashed from PoolA:

    • PoolA: 100 Value, 200 Shares
    • PoolB: 0 Value, 0 Shares
    • SuperPool: 200 Shares in PoolA (worth 100 Value), 0 Shares in PoolB
  4. Final state:

    • SuperPool share value = 100 / 200 = 0.5 Value per share

In both scenarios, the final value per share in the SuperPool is 0.5 Value. the attacker's actions did not result in any additional loss or gain ,The attacker essentially wasted gas on unnecessary transactions without affecting the overall outcome.

@sherlock-admin3 sherlock-admin3 added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Sep 17, 2024
@iamnmt
Copy link

iamnmt commented Sep 18, 2024

@elhajin

The example in this issue does not demonstrate the loss for the super pool, but the super pool will take a larger loss if it has more assets in the slashed pool.

Example when there is a loss

Scenario 1: No Attacker

Initial state:

  • PoolA: 200 Value, 200 Shares
  • PoolB: 100 Value, 100 Shares
  • SuperPool: 100 Shares in PoolA, 100 Shares in PoolB , superPoolShares is 200
  1. Bad debt of 100 Value slashed from PoolA:
    • PoolA: 100 Value, 200 Shares
    • PoolB: 100 Value, 100 Shares
    • SuperPool: 100 Shares in PoolA (worth 0.5 Value), 100 Shares in PoolB (worth 100 Value)
  2. Final state:
    • SuperPool total assets: 0.5 * 100 + 1 * 100 = 150 Value

Scenario 2: With Attacker

Initial state:

  • PoolA: 200 Value, 200 Shares
  • PoolB: 100 Value, 100 Shares
  • SuperPool: 100 Shares in PoolA, 100 Shares in PoolB , superPoolShares is 200
  1. Attacker deposits 100 Value to PoolA through SuperPool:

    • PoolA: 300 Value, 300 Shares
    • PoolB: 100 Value, 100 Shares
    • SuperPool: 200 Shares in PoolA (worth 200 Value), 100 Shares in PoolB (worth 100 Value) , superPoolShares is 300
  2. Attacker withdraws from PoolB:

    • PoolA: 300 Value, 300 Shares
    • PoolB: 0 Value, 0 Shares
    • SuperPool: 200 Shares in PoolA (worth 200 Value), 0 Shares in PoolB , superPoolShares is 200
  3. Bad debt of 100 Value slashed from PoolA:

    • PoolA: 200 Value, 300 Shares
    • PoolB: 0 Value, 0 Shares
    • SuperPool: 200 Shares in PoolA (worth 2/3 * 200 Value), 0 Shares in PoolB

Final state:

  • SuperPool total assets: 2/3 * 200 = 400/3 (approximately 133) Value

@0xdeth

Very low likelihood to have bad debt. Liquidating bad debt is a last resort and liquidators should have liquidated the position long before the bad debt was hit.

Sherlock's rules don't take likelihood into consideration. Referring to #487 for the pre-conditions for this attack to be possible.

Pool owner and reallocators are responsible to handle riskier pools, by changing deposit/withdraw orders, blocking deposits on riskier pools (setting poolCap = 0) or entirely removing riskier pools.

Yes the reallocation bots are responsible for move assets from the risky pools to the safer pools. But I believe saying the pool owner will set poolCap to zero is not a fair argument because for two reasons:

  • Why setting the poolCap the zero when there is no assets in it?
  • Setting poolCap and changing deposit/withdraw queue orders are the pool owner actions not the reallocation bots actions. The contest README provides only about the action of the reallocation bots

Are there any off-chain mechanisms or off-chain procedures for the protocol (keeper bots, arbitrage bots, etc.)?
Liquidator bots: maintain protocol solvency through timely liquidation of risky positions
Reallocation bots: used to rebalance SuperPool deposits among respective base pools

Having such pool order is not guaranteed and depositors trust owner and allocators to have the riskiest pool in the beginning of the withdraw queue, which makes the exploit path impossible in such cases.

Yes it is not guaranteed, therefore it is the pre-conditions. However, it is not guaranteed that the riskiest pool will get slashed first. Let say the order of the withdraw queue is [A, B,..]. The owner set A at the beginning because it is the riskiest pool. But B could get slashed first, and the attacker will move assets to B to cause loss to the super pool.

Moreover, refer to #487 for the attack path that levering flash loan. The attacker can move assets to a specific base pool when the pre-conditions are met.

@cvetanovv
Copy link
Collaborator

I agree with @iamnmt

The issue highlights a situation where SuperPool's queue order can be exploited by an attacker, causing losses to other depositors during bad debt liquidation.

Even though reallocation bots and pool owners are expected to manage risk, there is still a potential for harm.

Therefore, I believe this issue can be a Medium severity.

Planning to reject both escalation and leave the issue as is.

@Evert0x
Copy link

Evert0x commented Oct 2, 2024

Result:
Medium
Has Duplicates

@sherlock-admin2
Copy link
Contributor Author

sherlock-admin2 commented Oct 2, 2024

Escalations have been resolved successfully!

Escalation status:

@sherlock-admin3 sherlock-admin3 removed the Escalated This issue contains a pending escalation label Oct 2, 2024
@sherlock-admin4 sherlock-admin4 added the Escalation Resolved This issue's escalations have been approved/rejected label Oct 2, 2024
@sherlock-admin3 sherlock-admin3 added the Has Duplicates A valid issue with 1+ other issues describing the same vulnerability label Oct 11, 2024
@sherlock-admin3 sherlock-admin3 added Won't Fix The sponsor confirmed this issue will not be fixed and removed Will Fix The sponsor confirmed this issue will be fixed labels Oct 20, 2024
@sherlock-admin2
Copy link
Contributor Author

The protocol team fixed this issue in the following PRs/commits:
sentimentxyz/protocol-v2#340

@sherlock-admin3 sherlock-admin3 added Will Fix The sponsor confirmed this issue will be fixed and removed Won't Fix The sponsor confirmed this issue will not be fixed labels Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Escalation Resolved This issue's escalations have been approved/rejected Has Duplicates A valid issue with 1+ other issues describing the same vulnerability Medium A Medium severity issue. Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

10 participants