Closed sherlock-admin closed 4 months ago
1 comment(s) were left on this issue during the judging contest.
takarez commented:
valid: high(8)
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.
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.
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.
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
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
andprefundedRedeem
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.
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.
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.
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.
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.
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.
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.
The protocol team fixed this issue in PR/commit https://github.com/napierfi/napier-v1/pull/180.
Result: Medium Has duplicates
This is a duplicate of previously raised #120, so will be closing this issue and marking it a duplicate.
The Lead Senior Watson signed off on the fix.
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
Let's assume the following:
targetBufferPercentage
is set to 10%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:
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.
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:
Note that the deposits by other users will not mitigate this issue due to the following reasons:
prefundedDeposit
andprefundedRedeem
functions to carry out this attack as they are still accessible after maturity.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:
prefundedDeposit
andprefundedRedeem
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