xiaoming90 - Withdrawal can be blocked

[sherlock-admin]

xiaoming90

high

Withdrawal can be blocked

Summary

Malicious users can block withdrawal, preventing protocol users from withdrawing their funds.

Vulnerability Detail

When the adaptor does not have sufficient buffer ETH, users cannot redeem their PT tokens from the Tranche. It will need to wait for the buffer to be refilled.

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/adapters/BaseLSTAdapter.sol#L156

File: BaseLSTAdapter.sol
146:     function prefundedRedeem(address recipient) external virtual returns (uint256, uint256) {
147:         uint256 shares = balanceOf(address(this));
148:         uint256 assets = previewRedeem(shares);
149: 
150:         if (shares == 0) return (0, 0);
151:         if (assets == 0) revert ZeroAssets();
152: 
153:         uint256 bufferEthCache = bufferEth;
154:         // If the buffer is insufficient, shares cannot be redeemed immediately
155:         // Need to wait for the withdrawal to be completed and the buffer to be refilled.
156:         if (assets > bufferEthCache) revert InsufficientBuffer();

Let's assume the following:

• targetBufferPercentage is set to 10%
• current scale is 1.0
• total assets are 90 ETH (9 ETH buffer + 81 staked ETH)
• total supply = 90 shares

Bob (malicious user) could use the following formula to solve for "Deposit" variable OR perform off-chain simulation to determine the right number of assets to deposit for the attack:

Current TotalAsset = 90 ETH

(Current TotalAsset + Deposit) * 0.1 - Deposit = 0
(90 ETH + Deposit) * 0.1 - Deposit = 0
Deposit = 10 ETH

Bob deposits 10 ETH. It will become 100 ETH (10 ETH buffer + 90 staked ETH), and the total supply will increase to 100 shares. Bob will receive 10 shares in return after the deposit.

Bob redeems his 10 shares, and the adaptor will send him 10 ETH, which depletes the entire amount of ETH buffer within the adaptor. Since there is no fee charged during deposit and withdraw on the adaptor, Bob will not lose any of the initial 10 ETH.

assets = 10 shares * current scale = 10 ETH

Since malicious Bob has depleted the ETH buffer, the rest of the Napier users cannot withdraw.

Bob will perform the above attacks within a single transaction to DOS Napier users, preventing them from withdrawing. The users can only withdraw when the rebalancer bot unstake the staked ETH to replenish the buffer. However, the issue is that in the worst-case scenario, it will take 5 days for the redemption to be processed before the adaptor can start claiming the ETH from LIDO.

Following is the withdrawal period (waiting time) for unstaking ETH:

• LIDO: Between 1-5 days. Note that if more people exit the staking queue in Ethereum, the waiting period will increase further.
• FRAX: The sum total of both entry and exit queues (Reference)

Note that the deposits by other users will not mitigate this issue due to the following reasons:

• After maturity, no one can issue/mint PT+YT anymore from Tranche, resulting in no new ETH flowing into the adaptor. Yet, the attacker can directly call the Adaptor's prefundedDeposit and prefundedRedeem functions to carry out this attack as they are still accessible after maturity.
• As time moves closer to maturity, there will be fewer deposits naturally.

Impact

Users are unable to withdraw. This attack is cheap to execute (gas fee would be a few bucks), but the negative consequence to the protocol is significant (e.g., block withdrawal for 5 days). To DOS Napier for a month, one could execute the attack 6 times every 5 days, and the total costs are still cheap for the attacker, short traders, or competitors (other protocols).

Marking this as a High issue as the impact is High and probability is High (due to ease of execution and cheap to execute)

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/adapters/BaseLSTAdapter.sol#L156

Tool used

Manual Review

Recommendation

Consider any of the following measures to mitigate the issues:

• Charge fee upon depositing and withdrawal
• Restrict the amount of withdrawal allowed based on the existing ETH buffer remaining on the adaptor. For instance, using the same example, if Bob has 10 shares, totalSupply is 100 shares and the current ETH buffer is 10 ETH, he should only be allowed to withdraw 1 ETH at the maximum, which is 10% of the current ETH buffer based on his portion of shares in the adaptor.
• Restrict access to prefundedDeposit and prefundedRedeem function only to the Tranche. This does not entirely prevent this attack, but it makes the attack more expensive due to the issuance fee charged during the deposit when performed via Tranche.

Duplicate of #120

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

valid: high(8)

[massun-onibakuchi]

Users are unable to withdraw. This attack is cheap to execute (gas fee would be a few bucks), but the negative consequence to the protocol is significant (e.g., block withdrawal for 5 days). To DOS Napier for a month, one could execute the attack 6 times every 5 days, and the total costs are still cheap for the attacker, short traders, or competitors (other protocols).

Indeed, such DoS is possible but it would not be a permanent DoS, given the DoS is not permanent as long as there are deposits again. We can prevent such a long-time DoS, like a month, by setting targetBufferPercentage to high like 95% or 100%. It would have a negative effect to the protocol but some of these mentioned impacts are overestimated.

[ydspa]

Escalate
This finding should be Medium, as it only meets the 1st condition of the following Sherlock rule for DoS:

DoS has two separate scores on which it can become an issue:
1.The issue causes locking of funds for users for more than a week.
2.The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive).
If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.

[sherlock-admin2]

Escalate
This finding should be Medium, as it only meets the 1st condition of the following Sherlock rule for DoS:

DoS has two separate scores on which it can become an issue:
1.The issue causes locking of funds for users for more than a week.
2.The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive).
If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.

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.

[MehdiKarimi81]

1 - Setting targetBufferPercentage to 100% doesn't solve the issue, it removes the main functionality of the protocol which is yield generating, to solve this issue properly there should be a mechanism that handles withdrawals in a queue.
2 - There is no deposit after maturity
3 - Setting bufferETH to 95% doesn't prevent the attack, an attacker can perform deposit and withdrawal repeatedly and still make bufferETH = 0
3 - Even before maturity attacker can use a bot to make bufferETH = 0 after each deposit and technically it can lead to a permanent DoS.
4 - If we decide to set targetBufferPercentage to 100% it means the protocol doesn't have any use case and it's another issue.

So I believe this is a High

[xiaoming9090]

Users are unable to withdraw. This attack is cheap to execute (gas fee would be a few bucks), but the negative consequence to the protocol is significant (e.g., block withdrawal for 5 days). To DOS Napier for a month, one could execute the attack 6 times every 5 days, and the total costs are still cheap for the attacker, short traders, or competitors (other protocols).

Indeed, such DoS is possible but it would not be a permanent DoS, given the DoS is not permanent as long as there are deposits again. We can prevent such a long-time DoS, like a month, by setting targetBufferPercentage to high like 95% or 100%. It would have a negative effect to the protocol but some of these mentioned impacts are overestimated.

As mentioned in the report, the deposits by others will not mitigate the issue entirely:

Note that the deposits by other users will not mitigate this issue due to the following reasons:

• After maturity, no one can issue/mint PT+YT anymore from Tranche, resulting in no new ETH flowing into the adaptor. Yet, the attacker can directly call the Adaptor's prefundedDeposit and prefundedRedeem functions to carry out this attack as they are still accessible after maturity.
• As time moves closer to maturity, there will be fewer deposits naturally.

Suppose the protocol team decides to set the targetBufferPercentage to high, like 95% or 100%, in an attempt to prevent the DOS before maturity. In that case, almost all or all of the users' invested/deposited assets (e.g., ETH) sit idle on the adaptor's contract. Those deposited ETH are not invested in the external market (e.g., stake to LIDO) to earn yield. This breaks the core earning mechanism of the protocol. The purpose of the vault/adaptor is to invest the deposited assets in the external market so that the Yield Token (YT) holders can earn yield. If, in an extreme case (targetBufferPercentage = 100%) where none of the assets is invested in the external market, YT holders receive no yield during this period, which is undesirable and defeats the purpose of users holding YT tokens in the first place.

[xiaoming9090]

Escalate This finding should be Medium, as it only meets the 1st condition of the following Sherlock rule for DoS:

DoS has two separate scores on which it can become an issue:
1.The issue causes locking of funds for users for more than a week.
2.The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive).
If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.

Following is the Sherlock's judging rules:

DoS has two separate scores on which it can become an issue:
1.The issue causes locking of funds for users for more than a week.
2.The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive).
If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly.

The first requirement is met, as shown in the report. For the second requirement, withdrawal is a time-sensitive function of any protocol as it involves the need for someone to withdraw their assets. When we speak of not time-sensitive functions, it means those functions, such as depositing, fetching/retrieving information, or distributing reward tokens. Thus, the second requirement is also met.

Thus, the issue is considered to be of high severity.

[cvetanovv]

This issue should stay High. Lead Watson has shown very well how with minimal effort(in terms of time and cost) can DoS an important function of the protocol.

Regarding the rules on whether withdrawal is a time-sensitive function for me, it is time-sensitive. This is one of the most important rules of any DeFi protocol. Everyone should be able to withdraw their funds whenever they want.

[Czar102]

The protocol can be sunset by setting targetBufferPercentage to a high value to mitigate this, hence I believe this can be considered a loss of non-critical functionality (a migration is needed, but no loss of funds) and it's a valid Medium.

Planning to accept the escalation and consider it a Medium severity issue.

[MehdiKarimi81]

The protocol can be sunset by setting targetBufferPercentage to a high value to mitigate this, hence I believe this can be considered a loss of non-critical functionality (a migration is needed, but no loss of funds) and it's a valid Medium.

Planning to accept the escalation and consider it a Medium severity issue.

This is the main functionality of the protocol, without this functionality protocol doesn't have any usecace, how it's a non-critical function, actually this can't be fix for this issue.

[xiaoming9090]

The protocol can be sunset by setting targetBufferPercentage to a high value to mitigate this, hence I believe this can be considered a loss of non-critical functionality (a migration is needed, but no loss of funds) and it's a valid Medium.

Planning to accept the escalation and consider it a Medium severity issue.

@Czar102 Migration means the protocol's contracts have to be upgraded. The ability to upgrade or migrate should not be used to reduce the severity of an issue during an audit. The risk rating should be based on the context that the in-scope contracts are immutable.

Also, it is quite a serious issue if the entire protocol has to be sunsetted to solve this issue. Protocol itself is the critical feature. If you are referring to only sunsetting the protocol's affected adaptor, the adaptor is also a critical feature of the protocol as it manages all interactions with external money markets and the core earning mechanism in the protocol. Without the adaptor, the protocol is broken.

[Czar102]

If the admins behave correctly, the impact of this issue is that the protocol needs to be sunset, but no funds are lost or locked (anymore).

The risk rating should be based on the context that the in-scope contracts are immutable.

I'm not considering an upgrade here.

The impact of "protocol can't fulfill its purpose but doesn't lose funds" is clearly a Medium severity one based on the docs.

I maintain my position, but planning to consult this judgment with team members.

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/napierfi/napier-v1/pull/180.

[Czar102]

Result:
Medium
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ydspa: accepted

[Czar102]

This is a duplicate of previously raised #120, so will be closing this issue and marking it a duplicate.

[sherlock-admin4]

The Lead Senior Watson signed off on the fix.