Open sherlock-admin4 opened 2 months ago
2 comment(s) were left on this issue during the judging contest.
0xmystery commented:
Lacks proof to substantiate the bug on an intended design
Hash01011122 commented:
Invalid, If this were to occur it would be admin error to add any other underlying asset
I think this is a valid finding. Medium is a reasonable severity imo.
@jeffywu this reinforces the need to value withdraw requests as if they were still the staked asset except in the very strict case where we know exactly what we will get upon unstaking and exactly when we will get it
@mystery0x
To simplify the issue described, let's illustrate it using the PendlePTKelpVault
.
Before initiating a withdrawal, the vault shares' value is calculated based on the rsETH/WETH price. After a withdrawal request, the calculation is based on the stETH/WETH price.
In a situation where, for some reason, the stETH price drops compared to rsETH, the user's shares' value will also drop after initiating the withdrawal request, leading to the position becoming unhealthy for both the user and the protocol.
In such a case, it is better to block the initiation of the withdrawal. The better operation for the user or the Notional protocol in such cases is to close the position via _executeInstantRedemption()
, which will redeem rsETH to WETH via TRADING_MODULE at a better price.
Escalate.
This is a valid medium issue.
Escalate.
This is a valid medium issue.
The escalation could not be created because you are not exceeding the escalation threshold.
You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.
Escalate.
Escalating this issue on behalf of the submitter. Please review the above comments. Thanks.
Escalate.
Escalating this issue on behalf of the submitter. Please review the above comments. Thanks.
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.
Agree with the escalation, planning to accept it and validate with medium severity. @mystery0x are there additional duplicates?
I believe it is unique.
Result: Medium Unique
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/leveraged-vaults/pull/95/files
The Lead Senior Watson signed off on the fix.
eeyore
Medium
Premature collateralization check in the
BaseStakingVault.initiateWithdraw()
function can leave accounts undercollateralizedSummary
The collateralization check is currently performed before the user action that impacts the account's collateralization.
Vulnerability Detail
The
initiateWithdraw()
function can affect the solvency of the account. During this process, tokens may be unwrapped and new tokens pushed into the withdrawal queue, altering the underlying tokens for which collateralization was initially checked. This can result in a different collateralization level than initially assessed.Additionally, this contradicts how Notional core contracts perform such checks, where they are always conducted as the final step in any user interaction.
Impact
The account may become undercollateralized or insolvent following the user action.
Code Snippet
https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/BaseStakingVault.sol#L250-L255
Tool used
Manual Review
Recommendation
Perform the account collateralization check after the
_initiateWithdraw()
function call: