Falconhoof - SFrxETHAdapter redemptionQueue waiting period can DOS adapter functions

[sherlock-admin2]

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 from FraxEtherRedemptionQueue 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 struct redemptionQueueState as redemptionQueueState.queueLengthSecs and is curently set to 1_296_000 Seconds or 15 Days; as recently as January however it was at 1_555_200 Seconds or 18 Days. View current setting by calling redemptionQueueState() here.

BaseLSTAdapter::requestWithdrawal() is an essential function which helps to maintain bufferEth at a defined, healthy level.
bufferEth is a facility which smooth running of redemptions and deposits.

For redemptions; it allows users to redeem underlying without having to wait for any period of time.
However, redemption amounts requested which are less than bufferEth will be rejected as can be seen below in BaseLSTAdapter::prefundedRedeem().
Further, there is nothing preventing redemptions from bringing bufferEth all the way to 0.

    function prefundedRedeem(address recipient) external virtual returns (uint256, uint256) {
        // SOME CODE

        // If the buffer is insufficient, shares cannot be redeemed immediately
        // Need to wait for the withdrawal to be completed and the buffer to be refilled.
>>>     if (assets > bufferEthCache) revert InsufficientBuffer();

        // SOME CODE
    }

For deposits; where bufferEth is too low, it keeps user deposits in the contract until a deposit is made which brings bufferEth 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.

    function prefundedDeposit() external nonReentrant returns (uint256, uint256) {
    // SOME CODE
>>>     if (targetBufferEth >= availableEth + queueEthCache) {
>>>         bufferEth = availableEth.toUint128();
            return (assets, shares);
        }
    // SOME CODE
    }

Impact

If the SFrxETHAdapter experiences a large net redemption, bringing bufferEth significantly below targetBufferEth, 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 if redemptions > deposits, the bufferEth can be brought down to 0 which will mean a complete DOSing of the prefundedRedeem() function.

During the wait period too; if redemptions >= deposits, no new funds will be staked in FRAX 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 bring bufferEth 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

totalAssets() = (withdrawalQueueEth + bufferEth + sFrxETHBalance)
totalAssets() = (0 + 10 + 90)

Net Redemption 5 wETH reduces bufferEth so rebalancer makes Withdrawl Request of 4.5 wETH to bring bufferEth to 10% (9.5 wEth)

totalAssets() = (4.5 + 5 + 85.5)

During the wait period, continued Net Redemption reduces bufferEth further requiring another withdrawl request by rebalancer for 4.05 wEth

totalAssets() = (0 + 4.5 + 85.5)

Tool used

Foundry Testing
Manual Review

Recommendation

Consider adding a function allowing the rebalancer call earlyBurnRedemptionTicketNft() in FraxEtherRedemptionQueue.sol when there is a necessity.
This will allow an immediate withdrawal for a fee of 0.5%; see function here

[massun-onibakuchi]

We are aware of the situation. Therefore, we plan to set the TARGET BUFFER PERCENTAGE passively.

[nevillehuang]

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

[sherlock-admin2]

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.

[Qormatic]

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.

[nevillehuang]

@Qormatic You are correct, adjusted my escalation.

[massun-onibakuchi]

@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.

[massun-onibakuchi]

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.

[Qormatic]

@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.

[massun-onibakuchi]

@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.

[Qormatic]

@massun-onibakuchi why can't user call redeemWithYT(); there's no expired modifier on it?

[massun-onibakuchi]

@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.

[Banditx0x]

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.

[xiaoming9090]

@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.

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 (#89).

[xiaoming9090]

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 #89), this issue would not have occurred in the first place. Thus, this is not related to the design of the protocol.

[xiaoming9090]

@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.

[cvetanovv]

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.

[Czar102]

I agree with the escalation. Planning to consider this a duplicate of #89.

[Czar102]

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.

[nevillehuang]

@Czar102 So this is a dupe of #89? Applying changes but reject escalation correct?

[Czar102]

Yes, I edited my comment to be clearer. Technically, will consider #89 to be a duplicate of this issue.

[Czar102]

Result:
Medium
Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: rejected