xiaoming90 - Users are unable to collect their yield if tranche is paused

[sherlock-admin]

xiaoming90

medium

Users are unable to collect their yield if tranche is paused

Summary

Users are unable to collect their yield if Tranche is paused, resulting in a loss of assets for the victims.

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 admin of the protocol has the ability to pause the Tranche contract, and no one except for the admin can unpause it. If a malicious admin paused the Tranche contract, the users will not be able to collect their yield earned, leading to a loss of assets for them.

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L605

File: Tranche.sol
603:     /// @notice Pause issue, collect and updateUnclaimedYield
604:     /// @dev only callable by management
605:     function pause() external onlyManagement {
606:         _pause();
607:     }
608: 
609:     /// @notice Unpause issue, collect and updateUnclaimedYield
610:     /// @dev only callable by management
611:     function unpause() external onlyManagement {
612:         _unpause();
613:     }

The following shows that the collect function can only be executed when the system is not paused.

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L399

File: Tranche.sol
399:     function collect() public nonReentrant whenNotPaused returns (uint256) {
400:         uint256 _lscale = lscales[msg.sender];
401:         uint256 accruedInTarget = unclaimedYields[msg.sender];

Impact

Users are unable to collect their yield if Tranche is paused, resulting in a loss of assets for the victims.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/napier-v1/src/Tranche.sol#L605

Tool used

Manual Review

Recommendation

Consider allowing the users to collect yield even when the system is paused.

[nevillehuang]

Escalate

As mentioned by the watson, any issue that can causes a possible DoS/loss of funds by protocols admin not arising from external contract pauses/emergency withdrawals should be a valid medium severity issues due to centralization risks. In this case, protocol admins can block collection of yield permanently. In fact, it also blocks reinvestment of yield via issue()

[sherlock-admin2]

Escalate

As mentioned by the watson, any issue that can causes a possible DoS/loss of funds by protocols admin not arising from external contract pauses/emergency withdrawals should be a valid medium severity issues due to centralization risks. In this case, protocol admins can block collection of yield permanently. In fact, it also blocks reinvestment of yield via issue()

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 the escalation by Nevi. The report highlighted a way for the admin to pause the contract, resulting in the users not being able to collect their yield earned, leading to a loss of assets for them. Per the contest rules, such an issue is considered valid.

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

RESTRICTED

[cvetanovv]

The reason I left it invalid is that I consider the pausing mechanism a design decision.

Also, they have written in the readme that it is acceptable to have contracts pausing. Yes, I know this applies to External integrations, but I think it may be valid for them as well.

Apart from that pausing only stops users from collecting their rewards, doesn't mean the protocol will steal them. We have a sentence in the rules that gives the judge in this situation some flexibility to decide: "Please note that these restrictions must be explicitly described by the protocol and will be considered case by case."

[xiaoming9090]

The reason I left it invalid is that I consider the pausing mechanism a design decision.

Also, they have written in the readme that it is acceptable to have contracts pausing. Yes, I know this applies to External integrations, but I think it may be valid for them as well.

Apart from that pausing only stops users from collecting their rewards, doesn't mean the protocol will steal them. We have a sentence in the rules that gives the judge in this situation some flexibility to decide: "Please note that these restrictions must be explicitly described by the protocol and will be considered case by case."

I agree that having a pausing mechanism is a design choice by the protocol team. However, that does not mean the malicious admin will not use this pausing mechanism to block users from collecting their yields, causing harm to them.

The contest's README stated that pausing by external protocols is fine, but that does not mean that internal pausing is out-of-scope during the contest.

When users cannot collect their earned yield due to malicious admin activities, it is basically the same as a loss of assets for them. Loss of assets for either users or protocols is considered a valid issue here.

Note: Admin is restricted in this contest.

[Czar102]

I agree that this is a Medium severity issue, planning to accept the escalation. It is clear that when the protocol team considers the admins "RESTRICTED" and doesn't post the restrictions, the owners should not be able to cause losses to users.

@cvetanovv The pausing mechanism is a design decision, but when it starts to allow to cause loss of funds by an untrusted actor, then it becomes a vulnerability, too. They are fine with external integration pausing, meaning that they trust the protocols to do it with the good of users in mind. This has an odd relation with the fact that external admins are restricted, but luckily we are not considering external admins here.

[Czar102]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted