iamnmt - Wrong implementation of CuratedVault#reallocate when allocation.assets = 0 will cause unknown frontrunning donations can not be withdrawn

[sherlock-admin2]

iamnmt

Medium

Wrong implementation of CuratedVault#reallocate when allocation.assets = 0 will cause unknown frontrunning donations can not be withdrawn

Summary

Wrong implementation of CuratedVault#reallocate when allocation.assets = 0 will cause unknown frontrunning donations can not be withdrawn.

Root Cause

Per the Sherlock rules:

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

In the CuratedVault#reallocate function

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

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

1>      // Guarantees that unknown frontrunning donations can be withdrawn, in order to disable a market.
        uint256 shares;
        if (allocation.assets == 0) {
          shares = supplyShares;
2>        toWithdraw = 0;
        }
3>      DataTypes.SharesType memory burnt = pool.withdrawSimple(asset(), address(this), toWithdraw, 0);
        emit CuratedEventsLib.ReallocateWithdraw(_msgSender(), pool, burnt.assets, burnt.shares);
        totalWithdrawn += burnt.assets;
      } else {
        ...
      }
      ...

From the code comment at 1>, when allocation.assets = 0, all the assets of a pool must be withdrawn.

But when allocation.assets == 0, toWithdraw is set to zero (2>), then this value is passed to pool#withdrawSimple (3>). The withdrawSimple function will revert because the check validateWithdraw fails

https://github.com/sherlock-audit/2024-06-new-scope/blob/c8300e73f4d751796daad3dadbae4d11072b3d79/zerolend-one/contracts/core/pool/logic/ValidationLogic.sol#L97

  function validateWithdraw(uint256 amount, uint256 userBalance) internal pure {
>>  require(amount != 0, PoolErrorsLib.INVALID_AMOUNT);
    require(amount <= userBalance, PoolErrorsLib.NOT_ENOUGH_AVAILABLE_USER_BALANCE);
  }

As a result, the reallocate function will revert when allocation.assets = 0.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

An attacker still can front-run deposit to a pool. The allocator can not withdraw all the funds from a pool because the reallocate function will revert when allocation.assets = 0 will revert.

Impact

Because the reallocate function will revert when allocation.assets = 0, the protocol can not withdraw all the unknown frontrunning donations, in order to disable a market.

PoC

No response

Mitigation

Fix the implementation of reallocate when allocation.assets = 0

        uint256 shares;
        if (allocation.assets == 0) {
          shares = supplyShares;
-         toWithdraw = 0;
+         toWithdraw = type(uint256).max;
        }

Duplicate of #434