-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
Comments
1 comment(s) were left on this issue during the judging contest. Honour commented:
|
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
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
Escalate This report should be valid and is not a duplicate of #399 which is about To address the comment left on #193 that was referenced on this report:
I believe there is a flaw in this reasoning.
It's true the curator sets the 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. |
@Nihavent The impact is very similar to issues #337 and #431. You can see this comment: #337 (comment) I see no reason for it to be any different here. |
@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. This issue has nothing to do with EIP compliance. For reference this issue has the same root cause and impact as sherlock-audit/2024-08-sentiment-v2-judging#178 |
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 |
This is not true as I explained here #433 (comment) 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 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. |
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 |
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:
We can make inferences from the fact that these two caps 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:
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. |
I also believe this issue is not worth a medium severity, |
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. |
I believe this issue should be Low severity. While it could lead to some deposit reverts or inefficiencies in the Additionally, there is no direct loss of funds. For these reasons, I believe that this issue is of low severity. Planning to reject the escalation and leave the issue as is. |
Hi, thanks for consideration of the issue. It has been shown that this issue can occur regardless of the most reasonable admin setCap value. This is because the available liquidity in underlying pools can change quickly. For this parameter to be used in a way to almost completely avoid this issue, the cap would need to be set extremely low which defeats the purpose of the pool being in the supply queue. Additionally, as I explained in detail here the setCap functionality should not and will not be used to reduce the likelihood of this issue. If the protocol didn’t want curators to be able to freely set caps, the parameter wouldn’t exist and vaults would inherit the available caps from the underlying pools. As it stands, with admins freely setting their optimal caps as per the design, there is a bug in the implementation. |
This issue is a low severity There is no loss of funds or broken functionality or any substantial impact, the only issue here is a short/temporary inconvenience for the user that can be immediately and easily fixed by admin adjusting the cap or reordering the queue if necessary |
This issue is а valid Low severity. Here are the rules for Medium severity:
In this issue, there is neither loss of funds nor broken functionality. My decision to reject the escalation remains. |
If this issue does not have enough impact for medium severity, how is this issue a valid medium? sherlock-audit/2024-08-sentiment-v2-judging#178 I understand that reports will be judged on a case-by-case basis, but there is no meaningful difference between the protocols with respect to this issue, and the submitted issues are the same. Therefore for consistency if there is enough impact in that issue for medium, there must be enough impact in this issue for medium.
|
@Nihavent You are right about that. As you can see in the comments, I was hesitant about having broken functionality. After the protocol comment, then I decided it was Medium because it was important for them to have the function not revert. In this contract( Planning to accept the escalation and make this issue Medium. |
Hi @cvetanovv 193 is also a duplicate of this issue. |
Thanks for the mention. I will also duplicate #193 to this issue. |
Result: |
Escalations have been resolved successfully! Escalation status:
|
Nihavent
Medium
CuratedVaultSetters::_supplyPool()
does not consider the pool cap of the underlying pool, which may causedeposit()
to revert or lead to an unintended reordering ofsupplyQueue
Summary
The curated vault's
_supplyPool()
function deposits assets into the underlying pools insupplyQueue
. 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, someCuratedVault::deposit()
transactions will revert due to running out of pools to deposit to, or the liquidity will be allocated to thesupplyQueue
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 ofassets
andsupplyCap - supplyAssets
(which is the 'available curated pool cap' for this pool).However, the function does not consider an underlying pool's
supplyCap
. Underlying pools have their ownsupplyCap
which will causesupply()
calls to revert if they would put the pool over it'ssupplyCap
:Also note that the
pool::supplySimple()
call in_supplyPool()
is wrapped in a try/catch block, so if apool::supplySimple()
call were to revert, it will just continue to the next pool.Internal pre-conditions
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'ssupplyCap
.External pre-conditions
No response
Attack Path
supplyQueue
.supplyCap
of 100e18 and is currently at 99e18.supplyCap
of 100e18 and is currently at 99e18.deposit()
on the curated vault with a value of 2e18.config[pool].cap
for either pool.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 toImpact
Deposit Reverts
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
config[pool].cap
, but would exceed the limits an underlying pool insupplyQueue
. 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 thesupplyQueue
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(); }
The text was updated successfully, but these errors were encountered: