sherlock-admin3 commented 1 month ago

Nihavent

Medium

`CuratedVaultSetters::_supplyPool()` does not consider the pool cap of the underlying pool, which may cause `deposit()` to revert or lead to an unintended reordering of `supplyQueue`

Summary

The curated vault's _supplyPool() function deposits assets into the underlying pools in supplyQueue. Whilst it considers the curated vault's cap for a given pool, it does not consider the underlying pool's internal cap. As a result, some CuratedVault::deposit() transactions will revert due to running out of pools to deposit to, or the liquidity will be allocated to the supplyQueue in a different order.

Root Cause

CuratedVaultSetters::_supplyPool() is called in the deposit flow of the curated vault. As shown below, it attempts to deposit the minimum of assets and supplyCap - supplyAssets (which is the 'available curated pool cap' for this pool).

  function _supplyPool(uint256 assets) internal {
    for (uint256 i; i < supplyQueue.length; ++i) {
      IPool pool = supplyQueue[i];

      uint256 supplyCap = config[pool].cap;
      if (supplyCap == 0) continue;

      pool.forceUpdateReserve(asset());

      uint256 supplyShares = pool.supplyShares(asset(), positionId);

      // `supplyAssets` needs to be rounded up for `toSupply` to be rounded down.
      (uint256 totalSupplyAssets, uint256 totalSupplyShares,,) = pool.marketBalances(asset());
@>    uint256 supplyAssets = supplyShares.toAssetsUp(totalSupplyAssets, totalSupplyShares);

@>    uint256 toSupply = UtilsLib.min(supplyCap.zeroFloorSub(supplyAssets), assets);

      if (toSupply > 0) {
        // Using try/catch to skip markets that revert.
        try pool.supplySimple(asset(), address(this), toSupply, 0) {
          assets -= toSupply;
        } catch {}
      }

      if (assets == 0) return;
    }

    if (assets != 0) revert CuratedErrorsLib.AllCapsReached();
  }

However, the function does not consider an underlying pool's supplyCap. Underlying pools have their own supplyCap which will cause supply() calls to revert if they would put the pool over it's supplyCap:

  function validateSupply(
    DataTypes.ReserveCache memory cache,
    DataTypes.ReserveData storage reserve,
    DataTypes.ExecuteSupplyParams memory params,
    DataTypes.ReserveSupplies storage totalSupplies
  ) internal view {
    ... SKIP!...

    uint256 supplyCap = cache.reserveConfiguration.getSupplyCap();

    require(
      supplyCap == 0
@>      || ((totalSupplies.supplyShares + uint256(reserve.accruedToTreasuryShares)).rayMul(cache.nextLiquidityIndex) + params.amount)
          <= supplyCap * (10 ** cache.reserveConfiguration.getDecimals()),
      PoolErrorsLib.SUPPLY_CAP_EXCEEDED
    );
  }

Also note that the pool::supplySimple() call in _supplyPool() is wrapped in a try/catch block, so if a pool::supplySimple() call were to revert, it will just continue to the next pool.

Internal pre-conditions

A CuratedVault::deposit() call needs to be within the limits of the curated vault's cap (config[pool].cap) but exceed the limits of the underlying pool's supplyCap.

External pre-conditions

No response

Attack Path

A curated pool has two pools in the supplyQueue.
The first underlying pool has an internal supplyCap of 100e18 and is currently at 99e18.
The first underlying pool has an internal supplyCap of 100e18 and is currently at 99e18.
A user calls deposit() on the curated vault with a value of 2e18.
The value does not exceed the curated vault's config[pool].cap for either pool.
The underlying call to Pool::supplySimple() will silently revert on both pools, and the entire transaction will revert due to running out of available pools to supply the assets to
As a result, no assets are deposits, despite the underlying pools having capacity to accept the 2e18 deposit between them.

Impact

Deposit Reverts

If a deposit would be able to be deposited across two or more underlying pools in the supplyQueue, but is too large to be added to any one of these underlying pools, the deposit will completely revert, despite the underlying pools having capacity to accept the deposit.

Inefficient reorder of supplyQueue

If a deposit amount is within the limits of the curated pool's config[pool].cap, but would exceed the limits an underlying pool in supplyQueue. Then the silent revert would skip this pool and attempt to deposit it's liquidity to the next pool in the queue. This is an undesired/inefficient reordering of the supplyQueue as a simple check on the cap of the underlying pool would reveal some amount that would be accepted by the underlying pool.

PoC

No response

Mitigation

Create an external getter for a pool's supply cap, similar to ReserveConfiguration::getSupplyCap() the next function should also scale the supply cap by the reserve token's decimals.

Then, add an extra check in CuratedVaultSetters::_supplyPool() as shown below.

  function _supplyPool(uint256 assets) internal {
    for (uint256 i; i < supplyQueue.length; ++i) {
      IPool pool = supplyQueue[i];

      uint256 supplyCap = config[pool].cap;
      if (supplyCap == 0) continue;

      pool.forceUpdateReserve(asset());

      uint256 supplyShares = pool.supplyShares(asset(), positionId);

      // `supplyAssets` needs to be rounded up for `toSupply` to be rounded down.
      (uint256 totalSupplyAssets, uint256 totalSupplyShares,,) = pool.marketBalances(asset());
      uint256 supplyAssets = supplyShares.toAssetsUp(totalSupplyAssets, totalSupplyShares);

      uint256 toSupply = UtilsLib.min(supplyCap.zeroFloorSub(supplyAssets), assets);

+     toSupply = UtilsLib.min(toSupply, pool.getSupplyCap(pool.getConfiguration(asset())) - totalSupplyAssets );

      if (toSupply > 0) {
        // Using try/catch to skip markets that revert.
        try pool.supplySimple(asset(), address(this), toSupply, 0) {
          assets -= toSupply;
        } catch {}
      }

      if (assets == 0) return;
    }

    if (assets != 0) revert CuratedErrorsLib.AllCapsReached();
  }

Duplicate of #399

sherlock-admin3 commented 1 month ago

1 comment(s) were left on this issue during the judging contest.

Honour commented:

Invalid: see comment on #193

Nihavent commented 2 weeks ago

Escalate

This report should be valid and is not a duplicate of https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/399 which is about maxWithdraw and maxRedeem being non-ERC4626 compliant due to a bug in _withdrawable(). On the other hand, this report describes that _supplyPool() ignoring the underlying pool cap can result in unexpectedly reverting deposits or an inefficient reordering of the supplyQueue.

To address the comment left on https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/193 that was referenced on this report:

"setCap is an admin operation and It can be expected that the curators will set (there's no reason to assume otherwise) a similar pool(vault) cap as the underlying pool"

I believe there is a flaw in this reasoning.

The supplyCap stored in CuratedVaultStorage::config[pool].cap is the cap of deposits into a given underlying pool from this CuratedVault.
The underlying pool cap stored in the DataTypes.ReserveConfigurationMap for the underlying pool describes the total deposits to this underlying pool from all sources (including but not limited to this CuratedVault).

It's true the curator sets the supplyCap for the curated vault, however they are not in control of other deposts to the underlying pool. So, attempting to set them to similar values will only prevent this issue as long as there are no other deposits in the underlying pool, which is not a valid assumption for a curator to make.

Therefore both impacts in this report can occur regardless of the admin-set value.

sherlock-admin3 commented 2 weeks ago

Escalate

This report should be valid and is not a duplicate of https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/399 which is about maxWithdraw and maxRedeem being non-ERC4626 compliant due to a bug in _withdrawable(). On the other hand, this report describes that _supplyPool() ignoring the underlying pool cap can result in unexpectedly reverting deposits or an inefficient reordering of the supplyQueue.

To address the comment left on https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/193 that was referenced on this report:

"setCap is an admin operation and It can be expected that the curators will set (there's no reason to assume otherwise) a similar pool(vault) cap as the underlying pool"

I believe there is a flaw in this reasoning.

The supplyCap stored in CuratedVaultStorage::config[pool].cap is the cap of deposits into a given underlying pool from this CuratedVault.

The underlying pool cap stored in the DataTypes.ReserveConfigurationMap for the underlying pool describes the total deposits to this underlying pool from all sources (including but not limited to this CuratedVault).

It's true the curator sets the supplyCap for the curated vault, however they are not in control of other deposts to the underlying pool. So, attempting to set them to similar values will only prevent this issue as long as there are no other deposits in the underlying pool, which is not a valid assumption for a curator to make.

Therefore both impacts in this report can occur regardless of the admin-set value.

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.

cvetanovv commented 1 week ago

@Nihavent The impact is very similar to issues #337 and #431.

You can see this comment: https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/337#issuecomment-2402013737

I see no reason for it to be any different here.

Nihavent commented 1 week ago

@cvetanovv the root cause is similar to the issues you referenced but the impact here is higher.

The maximum impact on the issues you linked is non compliance with the EIP which can break external integrations, as you mentioned this does not necessarily qualify as medium impact.

The maximum impact on this issue is reverting deposits when the pools have capacity to support the deposit (see attack path). This is an implementation bug which is broken contract functionality. Additionally, assets can be allocated to underlying pools in an order which differs from the depositQueue.

This issue has nothing to do with EIP compliance.

For reference this issue has the same root cause and impact as https://github.com/sherlock-audit/2024-08-sentiment-v2-judging/issues/178

cvetanovv commented 1 week ago

@Nihavent I agree that in this issue, the impact is one idea more serious, and we enter the category "broken contract functionality".

Planning to accept the escalation and remove the duplication with #399. I will duplicate this issue(#433) with #193 and #339. This issue will be the main.

0xjuaan commented 1 week ago

Hi @cvetanovv, please consider the following judging comment regarding why this issue is invalid.

Vault curators should not set a cap that is greater than the cap of underlying pools. It makes no sense to do so. For example if the underlying pool allows a max of 10e18, then vault curators should set a deposit cap that is less than or equal to 10e18. This issue requires vault curators to set a cap that is higher than the underlying pool's cap, so is invalid.

Nihavent commented 1 week ago

Hi @cvetanovv, please consider the following judging comment regarding why this issue is invalid.

Vault curators should not set a cap that is greater than the cap of underlying pools. It makes no sense to do so. For example if the underlying pool allows a max of 10e18, then vault curators should set a deposit cap that is less than or equal to 10e18. This issue requires vault curators to set a cap that is higher than the underlying pool's cap, so is invalid.

This is not true as I explained here https://github.com/sherlock-audit/2024-06-new-scope-judging/issues/433#issuecomment-2391351629

Imagine the super pool has 2 underlying pools each with an underlying cap of 10e18. The SuperPool admin sets their caps to 10e18 as you said. Each underlying pool receives deposits from other sources to the value of 9e18. Now a user trying to deposit 2e18 into the SuperPool will revert even though this deposit could be split across the two underlying pools.

Therefore carefully setting the SuperPool cap for a given pool is only an effective mitigation if there are 0 deposits from other sources in the underlying pool. As I previously mentioned this is an unrealistic assumption for anyone to make.

”This issue requires vault curators to set a cap that is higher than the underlying pool's cap, so is invalid.”

This is also incorrect because in the above example the SuperPool cap set for the two pools could be 5e18 and the issue still exists.

I believe this is straightforward but if you require a POC showing the issue when the SuperPool cap is < the underlying pool cap I can write one tomorrow.

0xjuaan commented 1 week ago

Yeah actually you're right @Nihavent, the issue can still occur. I still think that the issue would not exist if vault curators set an appropriate cap, and that is why Morpho decided to implement it this way. In the case that an event like the described one occurs, the curators can reduce the cap of the pool such that only 1e18 can be deposited into each pool, and this prevents the revert.

That is the reason why submitCap() has no timelock for reducing a pool cap, but does have a timelock for increasing the pool cap. It's because reducing pool caps should be able to happen to prevent the described issue.

Nihavent commented 1 week ago

Edit: Updated this comment with a more considered and complete response.

@0xjuaan thanks for acknowledging the example I provided which shows the issue is possible regardless of carefuly set admin values. I do concede this issue is less likely to occur (but not impossible) if the admin continuously sets conservatively low SuperPool caps for the underlying pools (relative to available caps in the underlying pools). However, I will now explain why I don't believe the SuperPool cap should or would ever be used for this purpose.

There are two relevant caps we're discussing:

The SuperPool cap which is the maximum allocation that a SuperPool will allocate to a given underlying pool, and
The underlying pool's cap which is the maximum liquidity from all sources that can be deposited into the pool

We can make inferences from the fact that these two caps exist:

The SuperPool cap (1) is set by curators to represent the ideal maximum allocation to an underlying pool. This is one of the risk management tools at the disposal of the SuperPool admins.
If the intended use case of the SuperPool cap (1) was to always have it set to the available cap in the underling pool (2), this could have been achieved programatically and in fact the parameter (1) wouldn't exist.

This comment suggests that a partial mitigation to this issue is to burden SuperPool curators with administrative work of continuously checking for a reduction in available cap in all underlying pools, and reducing the corresponding SuperPool cap to reflect this change. The evidence given for this is there is no timelock on reduction in superPool caps. I believe there are problems with this:

It completely reduces the purpose of the SuperPool cap from a risk management/capital allocation tool to an administrative task for curators to update (in order to prevent edge-case deposit reverts).
We can reasonably expect that curators would not update the cap in this way because:
- The effort / benefits tradeoff doesn't make sense fo them to use the parameter in this way. They would care more about achieving their ideal capital allocation than preventing deposit reverts into the SuperPool.
- It is a waste of gas to update it potentially several times per day per underlying pool
- It is logistically impractical to poll for reductions in available caps across all underlying pools, they would need a bot to do this and then backrun underlying pool supply() calls with submitCap() calls.
- Whilst the cap can be reduced without a timelock, it requires a timelock to increase. Therefore if they reduced it to protect against this issue, and then there is a withdrawal from the underlying pool, they can't increase the cap again quickly. Therefore, their pool loses the chance to deposit more liquidity and a different SuperPool that wasn't continuously reducing their cap gets to deposit this liquidity instead.

So whilst it’s technically possible to reduce the likelihood of this issue with admin actions, it is unreasonable to expect the admin to use the parameter for this purpose.

Honour-d-dev commented 1 week ago

I also believe this issue is not worth a medium severity, submitCap() is an admin operation and it has been shown that the protocol is designed in such a way that its easy for the admin to handle this if it ever happens (and the chances of it happening are vey slim if the admin sets a reasonable cap to begin with)

Nihavent commented 1 week ago

I also believe this issue is not worth a medium severity, submitCap() is an admin operation and it has been shown that the protocol is designed in such a way that its easy for the admin to handle this if it ever happens (and the chances of it happening are vey slim if the admin sets a reasonable cap to begin with)

I agree the issue is not going to occur frequently due to it being a bug that occurs in edge cases. To my knowledge the likelihood of the bug does not play a role in Sherlock’s judging rules.

Check my previous comment for an explanation as to why setCap would need to be misused to reduce the likihood of this bug.

It has also been shown that this issue can occur regardless of admin set values.

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

Nihavent - `CuratedVaultSetters::_supplyPool()` does not consider the pool cap of the underlying pool, which may cause `deposit()` to revert or lead to an unintended reordering of `supplyQueue` #433

`CuratedVaultSetters::_supplyPool()` does not consider the pool cap of the underlying pool, which may cause `deposit()` to revert or lead to an unintended reordering of `supplyQueue`

Summary

Root Cause

Internal pre-conditions

External pre-conditions

Attack Path

Impact

PoC

Mitigation

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

Nihavent - `CuratedVaultSetters::_supplyPool()` does not consider the pool cap of the underlying pool, which may cause `deposit()` to revert or lead to an unintended reordering of `supplyQueue` #433

CuratedVaultSetters::_supplyPool() does not consider the pool cap of the underlying pool, which may cause deposit() to revert or lead to an unintended reordering of supplyQueue

Summary

Root Cause

Internal pre-conditions

External pre-conditions

Attack Path

Impact

PoC

Mitigation

`CuratedVaultSetters::_supplyPool()` does not consider the pool cap of the underlying pool, which may cause `deposit()` to revert or lead to an unintended reordering of `supplyQueue`