000000 - Funds will always be stuck in a pool and unexpected reverts will occur upon reallocations

[sherlock-admin3]

000000

High

Funds will always be stuck in a pool and unexpected reverts will occur upon reallocations

Summary

Funds will always be stuck in a pool and unexpected reverts will occur upon reallocations

Vulnerability Detail

Allocators can call CuratedVault::reallocate() to move funds between pools. To do that, they provide array of MarketAllocation structs. That struct has an assets value which is used to show how much assets must be in a particular pool after the loop iteration. For example, if the struct has an assets value of 10 for pool with ID of 10, then the pool with ID of 10 must have 10 assets after the function call. To confirm that, we can see these 2 lines:

(uint256 supplyAssets, uint256 supplyShares) = _accruedSupplyBalance(pool);
uint256 toWithdraw = supplyAssets.zeroFloorSub(allocation.assets);

We fetch the assets we can withdraw from a pool (including the accrued interest) and then from that value we deduct the allocation.assets. If the supplyAssets are 10 and the allocation.assets are 4, toWithdraw will be 6 which would make the assets in the pool equal 4, as explained above. If toWithdraw is over 0, we end up here:

if (toWithdraw > 0) {
        if (!config[pool].enabled) revert CuratedErrorsLib.MarketNotEnabled(pool);

        // Guarantees that unknown frontrunning donations can be withdrawn, in order to disable a market.
        uint256 shares;
        if (allocation.assets == 0) {
          shares = supplyShares;
          toWithdraw = 0;
        }

        DataTypes.SharesType memory burnt = pool.withdrawSimple(asset(), address(this), toWithdraw, 0);
        emit CuratedEventsLib.ReallocateWithdraw(_msgSender(), pool, burnt.assets, burnt.shares);
        totalWithdrawn += burnt.assets;
      }

There, we have this check which as seen by the comment above it, guarantees that unknown frontrunning donations can be withdrawn:

if (allocation.assets == 0) {
          shares = supplyShares;
          toWithdraw = 0;
        }

As we learned earlier, allocation assets of 0 means that the pool must have 0 assets after that iteration. However, if we see what value toWithdraw gets, it is 0. In reality, it should be the total assets we can withdraw or type(uint256).max as that is a special value used upon withdrawing to get all of the assets we have. Firstly, a toWithdraw value of 0 will make the withdraw function in the pool revert as that is 0 shares which is not allowed. Secondly, the supplied funds accrue interest, thus we would have to predict exactly how much interest has been accrued at the time of the function execution in order to get all of the assets we are entitled to (as we don't set toWithdraw to the special value of type(uint256).max). This is pretty much impossible as the interest is accrued based on block.timestamp which is unpredictable. Thus, we would always be left with unclaimed interest when reallocating.

Impact

Funds will always be stuck in a pool and unexpected reverts will occur upon reallocations

Code Snippet

https://github.com/sherlock-audit/2024-06-new-scope/blob/c8300e73f4d751796daad3dadbae4d11072b3d79/zerolend-one/contracts/core/vaults/CuratedVault.sol#L248-L251

Tool used

Manual Review

Recommendation

- toWithdraw = 0;
+ toWithdraw = type(uint256).max;

The other option is to use the shares variable to determine the assets you can withdraw, the way Morpho does it

Duplicate of #434