Vault protocol fees can be bypassed when removing markets

Summary

When the market is removed/disabled, the total last asset is not deducted, causing the next action can not accrue protocol fees

Root Cause

The function CuratedVault#updateWithdrawQueue() allows allocators to update queue orders and to remove markets which is proposed to be removed. However the function does not deduct totalLastAsset, which tracks the total asset currently controlled by the vault. And at the next vault action, totalLastAsset > currentTotalAssets (current total asset only accounts for assets supplied to the markets in the withdraw queue) which behaves that vault does not have interest. => Protocol fees is bypassed and totalLastAsset is set to currentTotalAssets

Internal pre-conditions

Assets are supplied to the market ( with amount > 0 )
Lenders do not withdraw all supplies from the market
Allocator proposes to remove the market without considering the supplied assets

External pre-conditions

The supplied market has interest accrued

Attack Path

The market is set in supply queue
Lenders supply to the market by depositing to the vault
Interest accrued for the market
Allocator sets market cap to zero
Allocator submits to remove the market
Allocator updates withdraw queue to completely remove the market
Any vault actions could be done but the protocol fee is not accrued

Impact

Protocol fees is bypassed

PoC

Add this test to the test file test/vaults/ERC4626Test.sol

...
  function test_marketRemove() public {
    // creating a market with interest
    vm.startPrank(owner);
    vault.setFee(0.1e18);
    vm.stopPrank();
    _setCap(allMarkets[0], 50 ether);

    IPool[] memory supplyQueue = new IPool[](1);
    supplyQueue[0] = allMarkets[0];

    loanToken.mint(supplier, 100 ether);

    vm.prank(allocator);
    vault.setSupplyQueue(supplyQueue);

    vm.startPrank(supplier);
    vault.deposit(20 ether, supplier);
    vm.stopPrank();

    collateralToken.mint(borrower, 200 ether);

    vm.prank(supplier);
    allMarkets[0].supplySimple(address(loanToken), supplier, 10 ether, 0);

    vm.startPrank(borrower);
    allMarkets[0].supplySimple(address(collateralToken), borrower, 10 ether, 0);
    allMarkets[0].borrowSimple(address(loanToken), borrower, 5 ether, 0);

    skip(5 days);
    loanToken.approve(address(allMarkets[0]), 1000 ether);
    allMarkets[0].repaySimple(address(loanToken), 5 ether, 0);

    // remove market
    vm.startPrank(owner);
    vault.submitCap(allMarkets[0], 0);
    vault.submitMarketRemoval(allMarkets[0]);

    skip(vault.timelock());

    uint256[] memory indexes = new uint256[](1);
    indexes[0] = 0;
    uint256 lastTotalAsset1 = vault.lastTotalAssets();
    vault.updateWithdrawQueue(indexes);
    uint256 lastTotalAsset2 = vault.lastTotalAssets();
    assertEq(lastTotalAsset1, lastTotalAsset2); // total last asset does not change
    vm.stopPrank();

    _setCap(vault.supplyQueue(0), 50 ether);
    loanToken.mint(supplier, 30 ether);

    vm.startPrank(supplier);
    uint feeShare = vault.balanceOf(feeRecipient);
    vault.deposit(10 ether, supplier);
    assertEq(feeShare, vault.balanceOf(feeRecipient));
  }
...

Run the test test_marketRemove and the console shows:

Ran 1 test for test/forge/core/vaults/ERC4626Test.sol:ERC4626Test
[PASS] test_marketRemove() (gas: 1236524)

Mitigation

Deduct lastTotalAssets by the total amount supplied to the market to be removed

Duplicate of #448

sherlock-audit / 2024-06-new-scope-judging

0xc0ffEE - Vault protocol fees can be bypassed when removing markets #68