search

sherlock-audit / 2024-08-sentiment-v2-judging

1 stars 0 forks source link

sl1 - Lack of slippage protection during withdrawal in SuperPool and Pool contracts. #356

Open sherlock-admin3 opened 1 month ago

sherlock-admin3 commented 1 month ago

sl1

Medium

Lack of slippage protection during withdrawal in SuperPool and Pool contracts.

Summary

Lack of slippage protection in the SuperPool and Pool could lead to loss of user funds in an event of bad debt liquidation.

Vulnerability Detail

When a user who has deposited assets in one of the pools of the Pool.sol contract wishes to withdraw them, they can do so by calling withdraw(). Under normal conditions, user expects to receive the full deposited amount back or more if the interest accrues in the underlying pool. However, if the pool experiences bad debt liquidation, the totalAssets of the pool are reduced by the amount of bad debt liquidated and the exchange rate worsens. Pool.sol#L542-L547

// rebalance bad debt across lenders
pool.totalBorrowShares = totalBorrowShares - borrowShares;
// handle borrowAssets being rounded up to be greater than totalBorrowAssets
pool.totalBorrowAssets = (totalBorrowAssets > borrowAssets)
    ? totalBorrowAssets - borrowAssets
    : 0;
uint256 totalDepositAssets = pool.totalDepositAssets;
pool.totalDepositAssets = (totalDepositAssets > borrowAssets)   <<@
    ? totalDepositAssets - borrowAssets  <<@
    : 0;

When a user withdraws, if the pool experiences bad debt liquidation, while the transaction is pending in the mempool, they will burn more shares than they expected.

Consider the following scenario:

The same issue is present in the SuperPool contract, as the totalAssets() of the SuperPool is dependant on the total amount of assets in the underlying pools a SuperPool has deposited into.

SuperPool.sol#L180-L189

function totalAssets() public view returns (uint256) {
    uint256 assets = ASSET.balanceOf(address(this));
    uint256 depositQueueLength = depositQueue.length;
    for (uint256 i; i < depositQueueLength; ++i) {
        assets += POOL.getAssetsOf(depositQueue[i], address(this));
    }
    return assets;
}

Pool.sol#L218-L227

function getAssetsOf(
    uint256 poolId,
    address guy
) public view returns (uint256) {
    PoolData storage pool = poolDataFor[poolId];
    (uint256 accruedInterest, uint256 feeShares) = simulateAccrue(pool);
    return
        _convertToAssets(
            balanceOf[guy][poolId],
            pool.totalDepositAssets + accruedInterest,
            pool.totalDepositShares + feeShares,
            Math.Rounding.Down
        );
}

When redeeming in the SuperPool, a user will either burn more shares when using withdraw() or receive less assets when using redeem().

Impact

withdraw() in the Pool.sol and both redeem/withdraw in the SuperPool lack slippage protection, which can lead to users loosing funds in the even of bad debt liquidation.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/Pool.sol#L339-L372 https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/SuperPool.sol#L281-L286 https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/SuperPool.sol#L293-L298

Tool used

Manual Review

Recommendation

Introduce minimum amount out for redeem() function and maximum shares in for withdraw() function as means for slippage protection.

z3s commented 3 weeks ago

Slippage protection cannot circumvent bad debt

kazantseff commented 3 weeks ago

Escalate, This issue and #245 should be valid. Slippage protection is not used to circumvent bad debt, but to protect users from loosing value when bad debt occurs.

sherlock-admin3 commented 3 weeks ago

Escalate, This issue and #245 should be valid. Slippage protection is not used to circumvent bad debt, but to protect users from loosing value when bad debt occurs.

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.

ruvaag commented 1 week ago

This should be invalid and not a duplicate of #245 because the title and description are not coherent.

While the title talks about slippage during withdraw / deposits (which is a separate issue), the description talks about funds lost due to a bad debt liquidation which has nothing to do with slippage

kazantseff commented 1 week ago

For slippage to occur, the totalAssets of the pool must reduce and exchange rate worsen, this can happen during bad debt liquidation. Otherwise there won't be any slippage as the exchange rate will stay 1:1 and there won't be a need for slippage protection at all.

cvetanovv commented 1 week ago

Watson has identified an edge case where the user could receive fewer shares than expected during a bad debt liquidation.

In the event of a bad debt liquidation, the pool's total assets decrease, causing the exchange rate to worsen. If a user attempts to withdraw or redeem during this period, they can burn more shares or receive fewer assets than anticipated.

I am planning to accept the escalation and make this issue Medium.

samuraii77 commented 6 days ago

@cvetanovv, hello, could #292 be considered a duplicate? It is for a different functionality in a different part of the code but the root cause is the same and the impact is very similar.

cvetanovv commented 5 days ago

I can't duplicate them because they have nothing in common.

The problem here is that a user can get fewer tokens when using redeem() or withdrawal() in the event of a bad debt liquidation.

There is nothing like that mentioned in #292. Usually, bots liquidate, and even if a user does it, he is not obliged to do it. Moreover, everything is different. Different parts of the code. Different fix. The root cause here is the lack of slippage on the bad debt liquidation event. This root cause is missing at #292.

Therefore, I will not duplicate them, and my previous decision will remain.

WangSecurity commented 4 days ago

Result: Medium Has duplicates

sherlock-admin2 commented 4 days ago

Escalations have been resolved successfully!

Escalation status: