Open sherlock-admin2 opened 5 months ago
We are aware of the situation. Therefore, we plan to set the TARGET BUFFER PERCENTAGE passively.
Escalate, this is a duplicate of #89 given it shares the exact same root cause of depletion of eth buffer to cause dos in withdrawals
Escalate, this is a duplicate of #89 given it shares the exact same root cause of depletion of eth buffer to cause dos in withdrawals
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.
Malicious intent is not required for this to be an issue; which it will be as it will DOS user's ability to access funds for an extended period of time. As noted in the issue; the withdrawal design aims to enable users withdraw their funds from Napier immediately without having to wait in the LST protocols withdrawal queue so this issue breaks the design of the protocol.
For redemptions; it allows users to redeem underlying without having to wait for any period of time.
Further; as per Sherlock docs; a DOS can be considered valid if users funds are locked up for more than a week. Given that, in this case, users funds would be locked up for 15 days it more than meets that threshold.
Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue: The issue causes locking of funds for users for more than a week.
@Qormatic You are correct, adjusted my escalation.
@nevillehuang it is expected that most of users will basically withdraw after the maturity because redemption of principal token can be only allowed after the maturity. ( redemptions
>> deposits
: after the maturity. redemptions
<< deposits
: before the maturity. ). so, considering the redemption queue time, we're going to set targetBufferPercentage to higher one over time and before bufferEth
derecase or 2 weeks before the the maturity, we will request withdrawal because our team knows it'd take 2 weeks to unstake ETH before this audit. The DoS can be caused by mismanagement. We believe that using waiting time as the basis for the claim does not reflect the actual situation.
Malicious intent is not required for this to be an issue; which it will be as it will DOS user's ability to access funds for an extended period of time.
I think this finding needs unlikely assumption. To borrow a phrase, this issue requires malicious intent.
@massun-onibakuchi I don't think it's an unlikely assumption that users would want to withdraw funds before maturity; else why would you provide them with zero wait functionality to do so?
And the Admin team may have a plan to manage a best case scenario but the DOS vulnerability would be caused by user actions outside the admin team's control and would negatively impact other users by locking up their funds for an extended period of time.
Also i dont think its fair post-contest to introduce additional context about off-chain withdrawl & buffer management which wasn't included in the README.
@Qormatic why do you think it's not unlikely? From economical perspective, it is easily expected that above. Actually after the maturity deposits will be reverted by contract and only after the maturity, redemption of PT is allowed.
targetBufferPercentage
is state variable can be changed by setTargetBufferPerecntage()
, which intends the value can be adjusted properly after deployment. That's why it has setter function and not immutable.
@massun-onibakuchi why can't user call redeemWithYT(); there's no expired modifier on it?
@massun-onibakuchi why can't user call redeemWithYT(); there's no expired modifier on it?
@Qormatic This method requires users to holds the same amount of PT/YT.
This is clearly the design of the protocol. It is impossible to always be able to redeem all tokens from a protocol (like Napier) which stakes tokens into a staking protocol with a redemption queue/delay.
Napier is designed to reduce the likelihood of a withdrawal being delayed, but cannot permanantly remove it.
@nevillehuang it is expected that most of users will basically withdraw after the maturity because redemption of principal token can be only allowed after the maturity. (
redemptions
>>deposits
: after the maturity.redemptions
<<deposits
: before the maturity. ). so, considering the redemption queue time, we're going to set targetBufferPercentage to higher one over time and beforebufferEth
derecase or 2 weeks before the the maturity, we will request withdrawal because our team knows it'd take 2 weeks to unstake ETH before this audit. The DoS can be caused by mismanagement. We believe that using waiting time as the basis for the claim does not reflect the actual situation.
The protocol is designed to allow users to withdraw both before and after maturity. Before maturity, users can withdraw via the redeemWithYT
function. After maturity, the users can withdraw via the redeem
or withdraw
function. The report and its duplicate have shown that it is possible for malicious actors to DOS the user withdrawal, which is quite severe.
The above measures do not fully mitigate the issue. If the maturity period is a year, the targetBufferPercentage
will only be set to a high value when it is near maturity.
Thus, the targetBufferPercentage
has to be low at all other times during the one-year period. Otherwise, the protocol's core earning mechanism is broken. So, the malicious actor could still carry out the attack for the vast majority of the one-year period, resulting in users being unable to withdraw.
The approach to fully mitigate this issue is documented in my report (https://github.com/sherlock-audit/2024-01-napier-judging/issues/89).
This is clearly the design of the protocol. It is impossible to always be able to redeem all tokens from a protocol (like Napier) which stakes tokens into a staking protocol with a redemption queue/delay.
Napier is designed to reduce the likelihood of a withdrawal being delayed, but cannot permanantly remove it.
If appropriate fees and restrictions had been put in place (See the recommendation section of https://github.com/sherlock-audit/2024-01-napier-judging/issues/89), this issue would not have occurred in the first place. Thus, this is not related to the design of the protocol.
@massun-onibakuchi I don't think it's an unlikely assumption that users would want to withdraw funds before maturity; else why would you provide them with zero wait functionality to do so?
And the Admin team may have a plan to manage a best case scenario but the DOS vulnerability would be caused by user actions outside the admin team's control and would negatively impact other users by locking up their funds for an extended period of time.
Also i dont think its fair post-contest to introduce additional context about off-chain withdrawl & buffer management which wasn't included in the README.
Agree with this. Note that this DOS issue, which is quite severe, is not documented under the list of known issues of the Contest's README. Thus, it would only be fair for Watson to flag this issue during the contest. Also, the mitigation mentioned by the sponsor cannot be applied retrospectively after the contest.
I agree with @nevillehuang escalation. This issue can be a dup of 89. There I have left a comment on what I think of the report.
I agree with the escalation. Planning to consider this a duplicate of #89.
During the escalation period this issue was valid and #89 – invalid. Hence, the duplication of #89 <> #120 should be proposed on #89. There already was an escalation there that showed that the issue is valid, so there is only a single mistake made here – #89 is not a duplicate of #120, but invalid. Hence, one escalation should be accepted and it was the one on #89. This issue is not undergoing any changes.
Hence, planning to apply the suggestion to duplicate these two, but also reject the escalation here.
@Czar102 So this is a dupe of #89? Applying changes but reject escalation correct?
Yes, I edited my comment to be clearer. Technically, will consider #89 to be a duplicate of this issue.
Result: Medium Has duplicates
Falconhoof
medium
SFrxETHAdapter redemptionQueue waiting period can DOS adapter functions
Links
https://github.com/FraxFinance/frax-ether-redemption-queue/blob/17ebebcddf31b7780e92c23a6b440dc789e5ceac/src/contracts/FraxEtherRedemptionQueue.sol#L417-L461 https://github.com/FraxFinance/frax-ether-redemption-queue/blob/17ebebcddf31b7780e92c23a6b440dc789e5ceac/src/contracts/FraxEtherRedemptionQueue.sol#L235-L246 https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/napier-v1/src/adapters/BaseLSTAdapter.sol#L71-L139 https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/napier-v1/src/adapters/BaseLSTAdapter.sol#L146-L169
Summary
The waiting period between
rebalancer
address making a withdrawal request and the withdrawn funds being ready to claim fromFraxEtherRedemptionQueue
is extremely long which can lead to a significant period of time where some of the protocol's functions are either unusable or work in a diminished capacity.Vulnerability Detail
In
FraxEtherRedemptionQueue.sol
; the Queue wait time is stored in the state structredemptionQueueState
asredemptionQueueState.queueLengthSecs
and is curently set to1_296_000 Seconds
or15 Days
; as recently as January however it was at1_555_200 Seconds
or18 Days
. View current setting by callingredemptionQueueState()
here.BaseLSTAdapter::requestWithdrawal()
is an essential function which helps to maintainbufferEth
at a defined, healthy level.bufferEth
is a facility which smooth running of redemptions and deposits.For
redemptions
; it allows users to redeemunderlying
without having to wait for any period of time. However, redemption amounts requested which are less thanbufferEth
will be rejected as can be seen below inBaseLSTAdapter::prefundedRedeem()
. Further, there is nothing preventingredemptions
from bringingbufferEth
all the way to0
.For
deposits
; wherebufferEth
is too low, it keeps user deposits in the contract until a deposit is made which bringsbufferEth
above it's target, at which point it stakes. During this time, the deposits, which are kept in the adapter, do not earn any yield; making those funds unprofitable.Impact
If the
SFrxETHAdapter
experiences a large net redemption, bringingbufferEth
significantly belowtargetBufferEth
, the rebalancer can be required to make a withdrawal request in order to replenish the buffer. However, this will be an ineffective action given the current, 15 Day waiting period. During the waiting period ifredemptions > deposits
, the bufferEth can be brought down to0
which will mean a complete DOSing of theprefundedRedeem()
function.During the wait period too; if
redemptions >= deposits
, no new funds will be staked inFRAX
so yields for users will decrease and may in turn lead to more redemptions.These conditions could also necessitate the immediate calling again of
requestWithdrawal()
, given that withdrawal requests can only bringbufferEth
up to it's target level and not beyond and during the wait period there could be further redemptions.Code Snippet
Simple example with Yield on
sFrxETHBalance
ignored:Start off with 100 wETH deposited; 10 wETH
bufferEth
Net Redemption 5 wETH reduces bufferEth so rebalancer makes Withdrawl Request of 4.5 wETH to bring bufferEth to 10% (9.5 wEth)
During the wait period, continued Net Redemption reduces bufferEth further requiring another withdrawl request by rebalancer for 4.05 wEth
Tool used
Foundry Testing Manual Review
Recommendation
Consider adding a function allowing the rebalancer call
earlyBurnRedemptionTicketNft()
inFraxEtherRedemptionQueue.sol
when there is a necessity. This will allow an immediate withdrawal for a fee of0.5%
; see function here