xiaoming90 - Permissioned rebalancing functions leading to loss of assets

[sherlock-admin]

xiaoming90

medium

Permissioned rebalancing functions leading to loss of assets

Summary

Permissioned rebalancing functions that could only be accessed by admin could lead to a loss of assets.

Vulnerability Detail

Per the contest's README page, it stated that the admin/owner is "RESTRICTED". Thus, any finding showing that the owner/admin can steal a user's funds, cause loss of funds or harm to the users, or cause the user's fund to be struck is valid in this audit contest.

Q: Is the admin/owner of the protocol/contracts TRUSTED or RESTRICTED?

RESTRICTED

The following describes a way where the admin can block users from withdrawing their assets from the protocol

1. The admin calls the setRebalancer function to set the rebalance to a wallet address owned by them.

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

File: BaseLSTAdapter.sol
245:     function setRebalancer(address _rebalancer) external onlyOwner {
246:         rebalancer = _rebalancer;
247:     }

2. The admin calls the setTargetBufferPercentage the set the targetBufferPercentage to the smallest possible value of 1%. This will cause only 1% of the total ETH deposited by all the users to reside on the adaptor contract. This will cause the ETH buffer to deplete quickly and cause all the redemption and withdrawal to revert.

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

File: BaseLSTAdapter.sol
251:     function setTargetBufferPercentage(uint256 _targetBufferPercentage) external onlyRebalancer {
252:         if (_targetBufferPercentage < MIN_BUFFER_PERCENTAGE || _targetBufferPercentage > BUFFER_PERCENTAGE_PRECISION) {
253:             revert InvalidBufferPercentage();
254:         }
255:         targetBufferPercentage = _targetBufferPercentage;
256:     }

3. The owner calls the setRebalancer function again and sets the rebalancer address to address(0). As such, no one has the ability to call functions that are only accessible by rebalancer. The requestWithdrawal and requestWithdrawalAll functions are only accessible by rebalancer. Thus, no one can call these two functions to replenish the ETH buffer in the adaptor contract.
4. When this state is reached, users can no longer withdraw their assets from the protocol, and their assets are stuck in the contract. This effectively causes them to lose their assets.

Impact

Loss of assets for the victim.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

To prevent the above scenario, the minimum targetBufferPercentage should be set to a higher percentage such as 5 or 10%, and the requestWithdrawal function should be made permissionless, so that even if the rebalancer does not do its job, anyone else can still initiate the rebalancing process to replenish the adaptor's ETH buffer for user's withdrawal.

[sherlock-admin]

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

takarez commented:

invalid

[massun-onibakuchi]

We are aware of such issues. The owner account is set to governance or multisig. To prevent the target buffer from becoming too low, it is set to 10% by default. This value can be changed even after deployment. Additionally, executing rebalancing functions may reduce the scale of the adapter. Making such functions callable by anyone would, conversely, become a vulnerability. Considering this trade-off, we chose to make it a permissioned function.

[nevillehuang]

Escalate

Unsure why this issue was excluded. The issue is highlighting how a potentially malicious protocol admin can cause a permanent DoS on users for core functionalities such as redemptions and withdrawals. Given protocol admins are explicitly mentioned as restricted in the contest details, I believe this issue should be valid medium severity.

[sherlock-admin2]

Escalate

Unsure why this issue was excluded. The issue is highlighting how a potentially malicious protocol admin can cause a permanent DoS on users for core functionalities such as redemptions and withdrawals. Given protocol admins are explicitly mentioned as restricted in the contest details, I believe this issue should be valid medium severity.

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.

[xiaoming9090]

Agree with Nevi. This report and its duplicates highlighted that it is possible for a malicious admin to negatively impact the users. Thus, it should be valid as per Sherlock's contest rules as admin is "restricted" in the contest's README.

[ABDuullahi]

This should stay invalid i believe, the README says Rebalancer: An account can manage adapter and request withdrawal for liquid staking tokens. It can't steal funds. and this issue does not describe a situation where the role can steal funds.

[xiaoming9090]

4. users can no longer withdraw their assets from the protocol, and their assets are stuck in the contract

The ability for the malicious admin (with rebalancer role) to cause users to be unable to withdraw their assets from the protocol, and in turn lead to their assets being stuck, is sufficient for this issue to be valid.

[cvetanovv]

I disagree with the escalation. The reason the report is not valid is the first comment from the sponsor. As we can see this is a design decision. Other than that, one of the recommendations is that targetBufferPercentage should be higher. But this is a configuration value that can be changed. So I think the report should remain Low/Invalid.

[xiaoming9090]

I disagree with the escalation. The reason the report is not valid is the first comment from the sponsor. As we can see this is a design decision. Other than that, one of the recommendations is that targetBufferPercentage should be higher. But this is a configuration value that can be changed. So I think the report should remain Low/Invalid.

A malicious admin can set the targetBufferPercentage is configured to the lowest possible value, and then calls the setRebalancer function again and sets the rebalancer address to address(0). As such, no one has the ability to call functions that are only accessible by rebalancer. The requestWithdrawal and requestWithdrawalAll functions are only accessible by rebalancer. Thus, no one can call these two functions to replenish the ETH buffer in the adaptor contract.

When the ETH buffer is not replenished, no one can withdraw from the protocol. This is sufficient to show that it is possible for malicious admins to harm users by preventing them from withdrawing. Note that the admin is restricted in this contest.

[ABDuullahi]

I think sherlock rules stated that the restriction must be explicitly mentioned, and for this role, its that it cant steal funds, not to disrupt the claiming/withdrawal process

[nevillehuang]

@ABDuullahi This issue is initiated by the admin (owner of contracts), not the rebalancer.

[Czar102]

This issue is initiated by the admin (owner of contracts), not the rebalancer.

@nevillehuang is the rebalancer relevant to this issue at all?

If not, I am planning to consider this a Medium severity issue and accept the escalation, with similar reasons to the ones listed here: https://github.com/sherlock-audit/2024-01-napier-judging/issues/97#issuecomment-1997410230.

[nevillehuang]

@Czar102 Not relevant, the admin can set the rebalancer to an address they control and/or themselves and execute the DoS

[Czar102]

@nevillehuang @cvetanovv is this a correct and full list of duplicates?

11, #21, #26, #119

[Czar102]

Result: Medium Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted