Closed sherlock-admin3 closed 4 months ago
Escalate.
The issue is that the canTriggerExtraStep
view function would not be accurate. It will signal to the caller that the triggerExtraStep
function can be executed earlier than it should. However, when the caller executes the triggerExtraStep
function, it will revert because the withdrawal delay has not passed yet under certain scenarios. The caller can simply call the triggerExtraStep
function again later once the withdrawal delay has passed.
The impact of this issue is more of a loss of gas or opportunity for the caller rather than a loss of assets for the protocol or user. In Sherlock, a loss of gas or opportunity is judged as Low.
Also, the report did not highlight how this issue can directly lead to a loss of assets for the protocol or user. Thus, it does not meet the criteria of H/M findings.
Escalate.
The issue is that the
canTriggerExtraStep
view function would not be accurate. It will signal to the caller that thetriggerExtraStep
function can be executed earlier than it should. However, when the caller executes thetriggerExtraStep
function, it will revert because the withdrawal delay has not passed yet under certain scenarios. The caller can simply call thetriggerExtraStep
function again later once the withdrawal delay has passed.The impact of this issue is more of a loss of gas or opportunity for the caller rather than a loss of assets for the protocol or user. In Sherlock, a loss of gas or opportunity is judged as Low.
Also, the report did not highlight how this issue can directly lead to a loss of assets for the protocol or user. Thus, it does not meet the criteria of H/M findings.
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.
I will response to each of your points :
The issue is that the canTriggerExtraStep view function would not be accurate. It will signal to the caller that the triggerExtraStep function can be executed earlier than it should. However, when the caller executes the triggerExtraStep function, it will revert because the withdrawal delay has not passed yet under certain scenarios. The caller can simply call the triggerExtraStep function again later once the withdrawal delay has passed.
canTriggerExtraStep
returns true, if the time delay has not passed, the triggerExtraStep
function will not revert in the context of national integration, but the transaction will be reverted by Kelp. This demonstrates improper integration.canTriggerExtraStep
function is to allow calls only if the request can be finalized. Additionally, the triggerExtraStep
function does not check for block delay, further indicating a flawed implementation. The impact of this issue is more of a loss of gas or opportunity for the caller rather than a loss of assets for the protocol or user. In Sherlock, a loss of gas or opportunity is judged as Low.
Also, the report did not highlight how this issue can directly lead to a loss of assets for the protocol or user. Thus, it does not meet the criteria of H/M findings.
I see that there's indeed an inconsistency, but I don't see it qualifying for Medium severity. As correctly mentioned, no funds are at risk, even the lock-up of funds and the funds will be withdrawn when the timelock passes. Hence, I don't see this issue qualifying for medium severity.
Planning to accept the escalation and invalidate the report.
I see that there's indeed an inconsistency, but I don't see it qualifying for Medium severity. As correctly mentioned, no funds are at risk, even the lock-up of funds and the funds will be withdrawn when the timelock passes. Hence, I don't see this issue qualifying for medium severity.
Planning to accept the escalation and invalidate the report.
Hi @WangSecurity,
I urge you to reconsider your decision for the following reasons:
The protocol has a dedicated library solely responsible for handling the withdrawal process. All integrations with other protocols are done correctly, except this one.
Not all findings at Sherlock point to a loss of funds or funds at risk. I will share a few examples with you here, some of which have been accepted by you.
napier-2024-01. This issue discusses a revert in certain conditions, meaning there is no loss of funds. Similar to this one, there is an inconsistency in integration.
napier-2024-05 This issue discusses a revert on deposit, which users can avoid by depositing the correct value. Similar to this one, user will withdraw after delay time.
napier--2024-05. This issue was accepted in escalation .
If there is no new rule that only issues involving loss of funds or funds at risk will be accepted at Sherlock, I think these references are enough to prove my point that this finding is indeed correct and should be considered as medium.
I agree that not all findings have to have the loss of funds to be valid. But I don't see what the impact here. I believe incorrect integration is a root cause, not the impact. The impact is that the users won't be able to withdraw their tokens for 24 hrs. That's why I don't see it as valid medium or high. Historical decisions are not sources of truth, but the reports you've sent, I believe have enough impact to be considered valid, unlike this one.
Planning to accept the escalation and invalidate the report.
Historical decisions are not sources of truth
Is this mentioned in Sherlock's rules? If yes, please share the reference here, and I will avoid using past reports as references. Otherwise, I cannot accept this as a valid argument.
but the reports you've sent, I believe have enough impact to be considered valid, unlike this one
Please review the first reference again. Its impact is exactly the same, with the only difference being that it pertains to a deposit with zero stake amount being reverted, whereas this one concerns a withdrawal due to block delay. There user will call again with correct amount is available to stake and here user will call again after block delay.
The impact is that the users won't be able to withdraw their tokens for 24 hrs
Additionally, there's a significant impact: users cannot determine the exact time their requests will be completed, and the National Protocol fails to accurately report this timing. This causes confusion and inefficiency, as all previous requests are mistakenly considered withdrawable before the last one. This undermines user trust and can lead to operational bottlenecks
.
If there is still any confusion you can discuss it with sponsor team.
Is this mentioned in Sherlock's rules? If yes, please share the reference here, and I will avoid using past reports as references. Otherwise, I cannot accept this as a valid argument.
Please check Sherlock Standards, line just before the DOS.
Please review the first reference again. Its impact is exactly the same, with the only difference being that it pertains to a deposit with zero stake amount being reverted, whereas this one concerns a withdrawal due to block delay. There user will call again with correct amount is available to stake and here user will call again after block delay.
The problem with the report you've sent is that the Napier's design was to allow deposits at any point and the report showed how this design is broken not allowing to deposit. Napier has a specific design which led to this finding being medium, while Notional is a different protocol with different design, that's why the historical decisions are not sources of truth.
Additionally, there's a significant impact: users cannot determine the exact time their requests will be completed, and the National Protocol fails to accurately report this timing. This causes confusion and inefficiency, as all previous requests are mistakenly considered withdrawable before the last one. This undermines user trust and can lead to operational bottlenecks.
I disagree the impact is significant, the users shouldn't be able to withdraw at this point and their funds won't be locked for more than 24 hrs. Moreover, the impact you mention is only user experience, which is also invalid based on Sherlock rules:
User experience and design improvement issues: Issues that cause minor inconvenience to users where there is no material loss of funds are not considered valid. Funds are temporarily stuck and can be recovered by the administrator or owner. Also, if a submission is a design opinion/suggestion without any clear indications of loss of funds is not a valid issue
Additionally, _canTriggerExtraStep
is internal function that is called only in canTriggerExtraStep
public view function, which is also invalid based on Sherlock rules:
Incorrect values in View functions are by default considered low
The decision remains the same, planning to reject the escalation and invalidate the issue.
Thank you for the details response. I will not talk about the reference Issue here due to Sherlock rules.
Incorrect values in View functions are by default considered low
Have you checked the report? The _canTriggerExtraStep
is only one part of my finding. The actual issue arises in the triggerExtraStep which is not view function. If this was only in view function than I would not reported it.
I would request you to discuss this with the sponsor, as the sponsor agreed with me on this issue during the audits.
Thank you for your time and support. and also there is a typo in you comment planning to reject the escalation and invalidate the issue
.
As I've said in my previous comment, this view function rule is not the reason for invalidation. The report is still about user experience. Hence, the decision remains the same, planning to accept the escalation and invalidate the issue (sorry for the typo), since the issue has no medium severity impact.
Result: Invalid Unique
The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/leveraged-vaults/pull/104
The Lead Senior Watson signed off on the fix.
aman
Medium
The integration with
Kelp:WithdrawManager
is not correctSummary
KelpLib:_canTriggerExtraStep
returns true if the withdrawal request at Kelp is permitted to proceed, but it does not account for the block delay imposed by Kelp for each withdrawal request.also inKelpCooldownHolder:triggerExtraStep
we only check for nonce and calls theWithdrawManager.completeWithdrawal(stETH);
Vulnerability Detail
For a
RSETH:ETH
withdrawal, the request will first be processed by the Kelp protocol, convertingRSETH
intoSTETH
. OnceSTETH
is received from Kelp, we trigger anSTETH:ETH
withdrawal request at LIDO. The issue lies in_canTriggerExtraStep
, which is solely responsible for returning true if the request at Kelp can be completed.And also intriggerExtraStep
which will revert.From the above code We can observed that it only check for the request Nonce if Nonce is less than
nextLockedNonce
then we return true. if we check aLRTWithdrawalManager:completeWithdrawal
function.it also check for block delay which is missing from our implementation.
Coded POC
Add following variable to PATH: ```javascript //.env in terminal export FORK_BLOCK=19691163 export FOUNDRY_PROFILE=mainnet export RPC_URL=MAINNET_URL ``` Add Following file as `Kelp.t.sol`: ```solidity // SPDX-License-Identifier: MIT pragma solidity 0.8.24; import "./Staking/harness/index.sol"; import {PendleDepositParams, IPRouter, IPMarket} from "@contracts/vaults/staking/protocols/PendlePrincipalToken.sol"; import {PendlePTOracle} from "@contracts/oracles/PendlePTOracle.sol"; import "@interfaces/chainlink/AggregatorV2V3Interface.sol"; import {WithdrawManager, stETH, LidoWithdraw, rsETH, KelpCooldownHolder, IWithdrawalManager} from "@contracts/vaults/staking/protocols/Kelp.sol"; import {PendlePTKelpVault} from "@contracts/vaults/staking/PendlePTKelpVault.sol"; import "forge-std/console.sol"; import {VaultRewarderTests} from "./SingleSidedLP/VaultRewarderTests.sol"; interface ILRTOracle { // methods function getAssetPrice(address asset) external view returns (uint256); function assetPriceOracle(address asset) external view returns (address); function rsETHPrice() external view returns (uint256); } ILRTOracle constant lrtOracle = ILRTOracle( 0x349A73444b1a310BAe67ef67973022020d70020d ); address constant unstakingVault = 0xc66830E2667bc740c0BED9A71F18B14B8c8184bA; contract Test_PendlePT_rsETH_ETH is BasePendleTest { function setUp() public override { FORK_BLOCK = 20033103; harness = new Harness_PendlePT_rsETH_ETH(); // NOTE: need to enforce some minimum deposit here b/c of rounding issues // on the DEX side, even though we short circuit 0 deposits minDeposit = 0.1e18; maxDeposit = 10e18; maxRelEntryValuation = 50 * BASIS_POINT; maxRelExitValuation = 50 * BASIS_POINT; maxRelExitValuation_WithdrawRequest_Fixed = 0.03e18; maxRelExitValuation_WithdrawRequest_Variable = 0.005e18; deleverageCollateralDecreaseRatio = 930; defaultLiquidationDiscount = 955; withdrawLiquidationDiscount = 945; super.setUp(); } function _finalizeFirstStep() private { // finalize withdraw request on Kelp address stETHWhale = 0x804a7934bD8Cd166D35D8Fb5A1eb1035C8ee05ce; vm.prank(stETHWhale); IERC20(stETH).transfer(unstakingVault, 10_000e18); vm.startPrank(0xCbcdd778AA25476F203814214dD3E9b9c46829A1); // kelp: operator WithdrawManager.unlockQueue( address(stETH), type(uint256).max, lrtOracle.getAssetPrice(address(stETH)), lrtOracle.rsETHPrice() ); vm.stopPrank(); } function _finalizeSecondStep() private { // finalize withdraw request on LIDO address lidoAdmin = 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84; deal(lidoAdmin, 1000e18); vm.startPrank(lidoAdmin); LidoWithdraw.finalize{value: 1000e18}( LidoWithdraw.getLastRequestId(), 1.1687147788880494e27 ); vm.stopPrank(); } function finalizeWithdrawRequest(address account) internal override { _finalizeFirstStep(); // trigger withdraw from Kelp nad unstake from LIDO WithdrawRequest memory w = v().getWithdrawRequest(account); PendlePTKelpVault(payable(address(vault))).triggerExtraStep( w.requestId ); _finalizeSecondStep(); } function getDepositParams( uint256 /* depositAmount */, uint256 /* maturity */ ) internal pure override returns (bytes memory) { PendleDepositParams memory d = PendleDepositParams({ // No initial trading required for this vault dexId: 0, minPurchaseAmount: 0, exchangeData: "", minPtOut: 0, approxParams: IPRouter.ApproxParams({ guessMin: 0, guessMax: type(uint256).max, guessOffchain: 0, maxIteration: 256, eps: 1e15 // recommended setting (0.1%) }) }); return abi.encode(d); } function test_Revert_When_Delay_Not_Passed_at_triggerExtraStep( uint8 maturityIndex, uint256 depositAmount, bool useForce ) public { depositAmount = uint256(bound(depositAmount, minDeposit, maxDeposit)); maturityIndex = uint8(bound(maturityIndex, 0, 2)); address account = makeAddr("account"); uint256 maturity = maturities[maturityIndex]; uint256 vaultShares = enterVault( account, depositAmount, maturity, getDepositParams(depositAmount, maturity) ); setMaxOracleFreshness(); vm.warp(expires + 3600); try Deployments.NOTIONAL.initializeMarkets( harness.getTestVaultConfig().borrowCurrencyId, false ) {} catch {} if (maturity < block.timestamp) { // Push the vault shares to prime totalVaultShares[maturity] -= vaultShares; maturity = maturities[0]; totalVaultShares[maturity] += vaultShares; } if (useForce) { _forceWithdraw(account); } else { vm.prank(account); v().initiateWithdraw(); } WithdrawRequest memory w = v().getWithdrawRequest(account); assertFalse( PendlePTKelpVault(payable(address(vault))).canTriggerExtraStep( w.requestId ) ); assertFalse( PendlePTKelpVault(payable(address(vault))) .canFinalizeWithdrawRequest(w.requestId) ); _finalizeFirstStep(); assertTrue( PendlePTKelpVault(payable(address(vault))).canTriggerExtraStep( w.requestId ) ); assertFalse( PendlePTKelpVault(payable(address(vault))) .canFinalizeWithdrawRequest(w.requestId) ); bytes4 selector = bytes4(keccak256("WithdrawalDelayNotPassed()")); vm.expectRevert(selector); PendlePTKelpVault(payable(address(vault))).triggerExtraStep( w.requestId ); } } contract Harness_PendlePT_rsETH_ETH is PendleStakingHarness { function getVaultName() public pure override returns (string memory) { return "Pendle:PT rsETH 27JUN2024:[ETH]"; } function getRequiredOracles() public view override returns (address[] memory token, address[] memory oracle) { token = new address[](2); oracle = new address[](2); // Custom PT Oracle token[0] = ptAddress; oracle[0] = ptOracle; // ETH token[1] = 0x0000000000000000000000000000000000000000; oracle[1] = 0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419; } function getTradingPermissions() public pure override returns ( address[] memory token, ITradingModule.TokenPermissions[] memory permissions ) { token = new address[](1); permissions = new ITradingModule.TokenPermissions[](1); // rsETH token[0] = 0xA1290d69c65A6Fe4DF752f95823fae25cB99e5A7; permissions[0] = ITradingModule.TokenPermissions({ // UniswapV3, EXACT_IN_SINGLE, EXACT_IN_BATCH allowSell: true, dexFlags: 4, tradeTypeFlags: 5 }); } function deployImplementation() internal override returns (address impl) { return address(new PendlePTKelpVault(marketAddress, ptAddress)); } function withdrawToken( address /* vault */ ) public pure override returns (address) { return stETH; } constructor() { marketAddress = 0x4f43c77872Db6BA177c270986CD30c3381AF37Ee; ptAddress = 0xB05cABCd99cf9a73b19805edefC5f67CA5d1895E; twapDuration = 15 minutes; // recommended 15 - 30 min useSyOracleRate = true; baseToUSDOracle = 0x150aab1C3D63a1eD0560B95F23d7905CE6544cCB; UniV3Adapter.UniV3SingleData memory u; u.fee = 500; // 0.05 % bytes memory exchangeData = abi.encode(u); uint8 primaryDexId = uint8(DexId.UNISWAP_V3); setMetadata(StakingMetadata(1, primaryDexId, exchangeData, false)); } } ``` run with command : `forge test --mt test_Revert_When_Delay_Not_Passed_at_triggerExtraStep -vvvvv` output : ```solidity ├─ [16807] vault::triggerExtraStep(599997995040149956827517125712971369603738040311 [5.999e47]) │ ├─ [16353] PendlePTKelpVault::triggerExtraStep(599997995040149956827517125712971369603738040311 [5.999e47]) [delegatecall] │ │ ├─ [15647] 0x6918D7322f9cE6e67E98546F769ef309efaAF7F7::triggerExtraStep() │ │ │ ├─ [15482] KelpCooldownHolder::triggerExtraStep() [delegatecall] │ │ │ │ ├─ [2576] 0x62De59c08eB5dAE4b7E6F7a8cAd3006d6965ec16::getUserWithdrawalRequest(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84, 0x6918D7322f9cE6e67E98546F769ef309efaAF7F7, 0) [staticcall] │ │ │ │ │ ├─ [1963] 0x79a0A901dBa2EE392709737D7542a1BC49Ca9AB2::getUserWithdrawalRequest(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84, 0x6918D7322f9cE6e67E98546F769ef309efaAF7F7, 0) [delegatecall] │ │ │ │ │ │ └─ ← 20346117805214044161 [2.034e19], 20629446294567039270 [2.062e19], 20033103 [2.003e7], 825 │ │ │ │ │ └─ ← 20346117805214044161 [2.034e19], 20629446294567039270 [2.062e19], 20033103 [2.003e7], 825 │ │ │ │ ├─ [1229] 0x62De59c08eB5dAE4b7E6F7a8cAd3006d6965ec16::nextLockedNonce(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84) [staticcall] │ │ │ │ │ ├─ [634] 0x79a0A901dBa2EE392709737D7542a1BC49Ca9AB2::nextLockedNonce(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84) [delegatecall] │ │ │ │ │ │ └─ ← 826 │ │ │ │ │ └─ ← 826 │ │ │ │ ├─ [10243] 0x62De59c08eB5dAE4b7E6F7a8cAd3006d6965ec16::completeWithdrawal(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84) │ │ │ │ │ ├─ [9647] 0x79a0A901dBa2EE392709737D7542a1BC49Ca9AB2::completeWithdrawal(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84) [delegatecall] │ │ │ │ │ │ ├─ [1187] 0x947Cb49334e6571ccBFEF1f1f1178d8469D65ec7::isSupportedAsset(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84) [staticcall] │ │ │ │ │ │ │ ├─ [592] 0xc5cD38d47D0c2BD7Fe18c64a50c512063DC29700::isSupportedAsset(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84) [delegatecall] │ │ │ │ │ │ │ │ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001 │ │ │ │ │ │ │ └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001 │ │ │ │ │ │ └─ ← WithdrawalDelayNotPassed() │ │ │ │ │ └─ ← WithdrawalDelayNotPassed() │ │ │ │ └─ ← WithdrawalDelayNotPassed() │ │ │ └─ ← WithdrawalDelayNotPassed() │ │ └─ ← WithdrawalDelayNotPassed() │ └─ ← WithdrawalDelayNotPassed() ```Impact
The
kelpLib._canTriggerExtraStep
returns true when the withdrawal request cannot be completed at Kelp due towithdrawalDelayBlocks
and also insidetriggerExtraStep
which will revert onWithdrawalDelayNotPassed
. It means that_canTriggerExtraStep
is not implemented as required byKelp:withdrawManager
.Code Snippet
https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L174 https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L75
Tool used
Manual Review , Foundry
Recommendation
Add following changes :