zarkk01 - reallocate function of CuratedVault can not withdraw fully from a Pool due to wrong handle of allocation.assets == 0.

[sherlock-admin4]

zarkk01

Medium

reallocate function of CuratedVault can not withdraw fully from a Pool due to wrong handle of allocation.assets == 0.

Summary

During the allocation of funds on a CuratedVault, allocator is supposed to pass allocation.assets = 0 if he wants to withdraw fully from a Pool but this case is not handled correctly.

Vulnerability Detail

When the allocator in a Pool wants to reallocate funds from one Pool to another, is supposed to call the reallocate function and pass the target assets he wants to have every Pool. We can see the implementation here :

  function reallocate(MarketAllocation[] calldata allocations) external onlyAllocator {
    uint256 totalSupplied;
    uint256 totalWithdrawn;

    for (uint256 i; i < allocations.length; ++i) {
      MarketAllocation memory allocation = allocations[i];
      IPool pool = allocation.market;

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

      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.
otalSupplied += suppliedAssets;
      }
    }

    if (totalWithdrawn != totalSupplied) revert CuratedErrorsLib.InconsistentReallocation();
  }

When he wants to move out every token from a Pool, he is supposed to put as parameter the 0 amount and must withdraw the whole balance of the CuratedVault from that Pool. However, as we can see in the implementation, when the allocation.assets == 0, the function instead of passing the type(uint256).max amount, it is passing the 0 amount which basically says to the Pool "withdraw nothing". This clearly goes against the intented behavior of the protocol.

Impact

The impact of this vulnerability is serious since this means that the CuratedVault is forced to let some amount on a Pool that the allocator wants to fully exit out of it. Also, it is clear that the reallocation of the funds during the call will not work and be executed as expected since the Pool will not be drained out with no funds.

Code Snippet

https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/vaults/CuratedVault.sol#L232C1-L276C4

Tool used

Manual Review

Recommendation

Consider making this change so to produce the intented functionality :

  function reallocate(MarketAllocation[] calldata allocations) external onlyAllocator {
    uint256 totalSupplied;
    uint256 totalWithdrawn;

    for (uint256 i; i < allocations.length; ++i) {
      // ...

      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;
+         toWithdraw = type(uint256).max;
        }

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

    if (totalWithdrawn != totalSupplied) revert CuratedErrorsLib.InconsistentReallocation();
  }

Duplicate of #434