0xDazai - SuperPool fails to correctly deposit into pools

[sherlock-admin2]

0xDazai

Medium

SuperPool fails to correctly deposit into pools

Summary

When a depositor calls SuperPool::deposit() the internal _deposit() is called, it checks if astTotalAssets + assets > superPoolCap , transfers the assets from msg.sender to superPool address , mints shares to receiver and then calls _supplyToPools().

SuperPool::_deposit()

    function _deposit(address receiver, uint256 assets, uint256 shares) internal {
        // assume that lastTotalAssets are up to date
        if (lastTotalAssets + assets > superPoolCap) revert SuperPool_SuperPoolCapReached();
        // Need to transfer before minting or ERC777s could reenter.
        ASSET.safeTransferFrom(msg.sender, address(this), assets);
        ERC20._mint(receiver, shares);
        _supplyToPools(assets);    <<<@
        lastTotalAssets += assets;
        emit Deposit(msg.sender, receiver, assets, shares);
    }

SuperPool::_supplyToPools()

    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;
            }
        }
    }

_supplyToPools() loops through all pools, depositing assets sequentially until the cap is reached. When it checks if the cap of the poolId is reached instead of comparing the total deposit assets amount of the poolId with the pool.poolCap to see if there is a free space for depositing into, it only compares the total assets deposited by the SuperPool address into the poolId with poolCapFor[poolId] mapping set by the owner of the SuperPool when the pool was added by calling addPool() and subtract the result with the wanted asset value for depositing.

Vulnerability Detail

When calculating if there is a free space for depositing into the poolId by calling uint256 assetsInPool = POOL.getAssetsOf(poolId, address(this)); it can return bigger value than the actual one left in the pool.poolCap , increasing the chances of deposit() function for the poolId to revert, unsuccessfully filling up the left space in the poolId before moving forward to the next poolId if there is any asset amount left.

Impact

Fails to correctly fill up assets into pools even if there is any free space to do so.

Code Snippet

    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;
            }
        }
    }

PoC

Lets look at the following example:

1. Owner of poolId = 1 creates the pool and sets poolCap = 2000 USDC
2. In SuperPool poolId = 1 is added to the contract with poolCapFor[poolId] = 1500 .
3. Alice deposits 1000 USDC to poolId = 1 by calling SuperPool.deposit().
   a) Now the poolCapFor[poolId] free space is 500 USDC.
   b) And poolCap free space for poolId = 1 is 1000 USDC.
4. Bob calls directly Pool.deposit() for poolId = 1 with 600 USDC , and poolCap free space for poolId = 1 is 400USDC.
5. John calls SuperPool.deposit() with 500 USDC and it will try to deposit into poolId = 1 because poolCapFor[poolId] free space = 500 , but poolCap free space = 400, the tx will revert for that poolId and will move forward and try to deposit into the next pool even when there is free space for 400 USDC .

Tool used

Manual Review

Recommendation

In Pool.sol add :

+    function getPoolCap(uint256 poolId) public view returns(uint256) {
+        return poolDataFor[poolId].poolCap;
+    }

And in SuperPool.sol

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

                if (assetsInPool < poolCapFor[poolId]) {
                uint256 supplyAmt = poolCapFor[poolId] - assetsInPool;
                if (assets < supplyAmt) supplyAmt = assets;
+                If(supplyAmt > capLeft){
+                    supplyAmt = capLeft;
                ASSET.forceApprove(address(POOL), supplyAmt);
+                } else {
+                    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;
            }
        }
    }

[S3v3ru5]

Isn't that the intention of SuperPool.poolCapFor[poolId]?

The poolCapFor in the superpool is to do with the super pool itself: Max amount the super pool wants to deposit it into a certain pool.

pool.PoolCap is max amount of assets in the pool.

The issue is clearly invalid. The issue is considering incorrect definition for state variable

[NicolaMirchev]

Escalate.
As per the above comment

[sherlock-admin3]

Escalate.
As per the 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.

[0xDazaII]

Isn't that the intention of SuperPool.poolCapFor[poolId]?

The poolCapFor in the superpool is to do with the super pool itself: Max amount the super pool wants to deposit it into a certain pool.

pool.PoolCap is max amount of assets in the pool.

The issue is clearly invalid. The issue is considering incorrect definition for state variable

Every BasePool has different settings which can incentivise users to or not to prefer to deposit into a BasePool ( e.g interest rate , rateModel and so on) . As its said in the docs ‘users who are accessing Sentiment UI has indirect interaction with base pools, Super Pools will take care of efficiently distributing user liquidity across multiple base pools for enhanced liquidity management. ‘ Having this in mind users who are using Sentiment UI fully rely on the correct functionality of the SuperPool contract.

Base Pools and SuperPools have a maximum poolCap set which shouldn’t be exceeded. Also BasePools can be used by multiple SuperPools or directly from their contract. These are the ways to deposit into a BasePool and eventually hit their cap depending of the cap value.

The issue I am describing is that if SuperPool.poolCapForId is not reached in the ‘deposit’ function the ‘_supplyToPools()’ can call ‘POOL.deposit()’ with bigger value than it is available into the BasePool and the tx will revert because ‘_supplyToPools()’ is not checking how much exactly enough space does the BasePool has before hitting its cap and in case when the free space into the BasePool is smaller than the value which is tried to be deposited the tx will revert instead of depositing the amount which is available into that BasePool and the rest ,if there is any , to be deposited into the next BasePool from the SuperPool queue.

[Tomiwasa0]

I recommend @S3v3ru5 to read the report well. With a basic understanding of the code, multiple Superpools, as explained by 0xDazall and others, can use the same pool.
The check should check the total deposited asset and compare with the cap not individual deposits with the cap

[S3v3ru5]

Sorry for misunderstanding.

So the issue is: The available free space in the base pool could be less than the supply amount and the POOL.deposit function might revert because of that. The _supplyToPools() function should try to deposit a max of available space in the base pool instead of the min(supplyAmount, poolCapFor[poolId] - supplyAmount).

[Tomiwasa0]

The root cause is the most important thing that we should note. For better mitigations, you can check through other duplicates, sometimes the best report is not the best. But we practically are to deposit the minimum between the available space in the main pool (considering other pools) and the supply amount.

[elhajin]

From the readme :

Please discuss any design choices you made.

The deposit and withdraw flows in the SuperPool sequentially deposit and withdraw from pools. This can be inefficient at times. We assume that the SuperPool owner will use the reallocate function to rebalance liquidity among pools.

[Tomiwasa0]

Thanks for the input @elhajin, kindly look through the code implementation. The same check flaw exists in the rebalance function. It was indeed an oversight that has been confirmed and will be fixed. We can't risk having an inefficient deposit, withdrawal and rebalance function, can we? Also, Feel free to read other duplicates also.

[cvetanovv]

Looks like we have an issue here and the deposit function is not working as it should.

@ruvaag what do you think?

[dhankx7]

Please reconsider the issue #602.

It clearly states the similar issue and should be grouped with this bug.

[ruvaag]

I think this is valid. The Base Pool cap should be taken into account.

the intended behavior is to deposit as much as possible in the pools, and this helps with that.

[cvetanovv]

The sponsor confirms that we have a issue here.

The issue is valid because the Base Pool cap should be considered when calculating the available deposit space in the pool.

I will also duplicate #602 to this issue.

Planning to reject the escalation and leave the issue as is. I will duplicate #602 to this issue.

[WangSecurity]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• EgisSecurity: rejected

[sherlock-admin2]

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