dhank - Protocol lets the asset to accumulate in the SuperPool contract without depsoting to available deposit pools.

[sherlock-admin2]

dhank

Medium

Protocol lets the asset to accumulate in the SuperPool contract without depsoting to available deposit pools.

Summary

The supplyToPools function is supposed to distribute the depositors funds accross the pools in an optimal way so that assets will not be accumulated in the superPool before filling the pools mentioned in the depositQueue.
But since we are not checking the basepoolCap mentioned in BasePool for all the queued pools, it will revert if an amount greater than the basepoolCap is deposited.
If that basePool is the last pool in the queue , even though the contract can deposit assets into that pool it will let it revert and store the assets in the contract itself hence being inefficent in allocation and as a result less interest is supplied for the depositors

Vulnerability Detail

In the _supplyToPools() , supplyAmt for a pool is calculated by comparing the available amount to reach the poolCapFor that pool and the remaining assets to be deposited.

If the comparison is satisfied then it will try depositng that amount to the pool.

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

    function _supplyToPools(uint256 assets) internal {
        uint256 depositQueueLength = depositQueue.length;
        for (uint256 i; i < depositQueueLength; ++i) {
            uint256 poolId = depositQueue[i];
            uint256 assetsInPool = POOL.getAssetsOf(poolId, address(this));

            if (assetsInPool < poolCapFor[poolId]) {
                uint256 supplyAmt = poolCapFor[poolId] - assetsInPool;
=>                if (assets < supplyAmt) supplyAmt = assets;
                ASSET.forceApprove(address(POOL), supplyAmt);


                // skip and move to the next pool in queue if deposit reverts
=>                try POOL.deposit(poolId, supplyAmt, address(this)) {
                    assets -= supplyAmt;
                } catch { }


                if (assets == 0) return;
            }
        }
    }

But the bug occurs , when the POOL.deposit(poolId, supplyAmt, address(this)) reverts because the supplyAmt < base poolCap mentioned by the underlying basePool owner. code

And the entire supplyAmt is then checked with the next pool in the deposit Queue.

Suppose we are in the last pool of the depositqueue , then the entire supplyAmt gets accumulated in the SuperPool contract goeswithout yielding any interest.

When protocol could have deposit basePoolCap - supplyAmount to that last pool it decided to let the entire supplyAmt to get accumulated in the superPool leading to very less optimal preference.

Impact

Protocol lets the asset to accumulated in the SuperPool contract even though they can be deposited in the pools mentioned in the QUEUE.

Code Snippet

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

Tool used

Manual Review

Recommendation

Check for the basePoolCap too.

Duplicate of #178

[sherlock-admin4]

1 comment(s) were left on this issue during the judging contest.

z3s commented:

Design Decision