Ethena Labs - Bughunter101's results

raymondfam marked the issue as sufficient quality report

raymondfam marked the issue as primary issue

Good catch.

raymondfam marked the issue as high quality report

FJ-Riveros (sponsor) acknowledged

fatherGoose1 marked the issue as satisfactory

fatherGoose1 marked the issue as selected for report

Hi, @fatherGoose1!

I think this issue should be marked as Low Severity issue (QA), because the given impact of this problem does not lead to financial losses (assets are not at risk).

Hey, @fatherGoose1, I think this issue should be Low Severity (QA) as well: (1) This is intended behavior based on code implementation: In the demonstrated attack scenario by warden, a) Bob can withdraw after 5 days, only because Bob called the cool-down function after admin set the cool-down to 5 days; b) Alice has to wait for 30 days, because Alice called the cool-down when the cool-down period is 30 days; Nothing is techinically wrong here and there is no exploit; (2) As the comment above said, there is no funds at risk, which also makes it a QA severity; In addition, the warden thinks this is unfair for Alice. I don't agree, because Alice agreed on a 30-day cool-down when she calls cooldownAssets. It's like a fixed-term bank deposit in which the term is 30-day.

@flowercrimson @radeveth @fatherGoose1 the contract has coolDown states which work like switches. You can toggle it on or off. Toggle off by setting coolDown duration to 0, toggle on by setting to non-zero. In the event that it is toggled off, the expected behaviour is that all withdrawals from the silo contract can be removed immediately but that doesnt happen. Dev intent about this can be confirmed from the comments here so thats why i think it is a valid issue.

I am changing this issue to QA. While the sponsor acknowledged and the functionality does not match the code comment (described in above comment), there is no actual impact here.

The warden lists the following scenario:

• The cool-down time set by the owner at the beginning is 30 days.
• Then Alice calls the cooldownAssets function and waits for the cool-down time
• After 2 days, the cool-down time set by the owner is 5 days.
• Bob calls the cooldownAssets function and then waits for the cool-down time
• However, Alice still needs to wait 28 days before calling unstake(), while Bob only needs 5 days, which is unfair. Will cause Alice's funds to be locked for no reason

In the above scenario explained by the warden, Alice can simply call cooldownAssets() again to set the cooldown time to the shorter 5 days. The staker can navigate away from the "unfair scenario" by cooling down once again.

fatherGoose1 changed the severity to QA (Quality Assurance)

@fatherGoose1 I believe this to be a Medium severity finding. As per your reply,

In the above scenario explained by the warden, Alice can simply call cooldownAssets() again to set the cooldown time to the shorter 5 days. The staker can navigate away from the "unfair scenario" by cooling down once again.

But this wouldn't be possible according in "Scenario 3" of the report #29 which is

Scenario 3:

• Current cooldownDuration = 90 days
• Zayn calls cooldownAssets to redeem some assets.
• cooldownEnd for Zayn is set to 90 days counting from the present timestamp.
• Admin changes cooldownDuration to 0 days.
• Zayn immediately calls unstake but the function reverts because now he has to wait for 90 days until cooldownEnd is achieved.

Here Zayn won't be able to call cooldownAssets() because of ensureCooldownOn modifier which would cause a revert.

95:   function cooldownAssets(uint256 assets, address owner) external ensureCooldownOn returns (uint256) {

33:   modifier ensureCooldownOn() {
34:       if (cooldownDuration == 0) revert OperationNotAllowed();
35:       _;
36:   }

Zayn's funds would be stuck in the silo contract until the 90 days are over.

Hi @Shubh0412!

You say:

"Here Zayn won't be able to call cooldownAssets() because of ensureCooldownOn modifier which would cause a revert."

Why should Zayn call the cooldownAssets() function again (for the second time) after he has redeemed his assets and started a cooldown period to claim his converted underlying asset? After 90 days, Zayn will should just to call unstake() to claim his converted underlying asset.

As mentioned in the comment by @flowercrimson:

In addition, the warden thinks this is unfair to Alice. I don't agree because Alice agreed to a 30-day cooldown when she called cooldownAssets(). It's similar to a fixed-term bank deposit with a 30-day term.

The same principle applies here.

I have actually explained this issue in my analysis report and why it is invalid/nc.

Hello @radeveth I read the analysis report. The scenario you mentioned there is the same as my above comment.

When the cooldown duration is disabled (set to 0), the users who have already called cooldownAssets() or cooldownShares() and sent their assets/shares to the Silo contract would not be able to call cooldownAssets()/cooldownShares() because of the ensureCooldownOn modifier.

Since cooldownDuration == 0 now, calling cooldownAssets()/cooldownShares() would revert with the error OperationNotAllowed.

According to the analysis report

After all, I observed that users who have already called cooldownAssets()/cooldownShares() can call the unstake() function again to retrieve their converted underlying asset.

How calling the unstake() again will the users be able to retrieve their converted asset? Calling unstake() will always revert with InvalidCooldown because of if (block.timestamp >= userCooldown.cooldownEnd) check. If a user has deposited when cooldownDuration was 90 days, then unless the 90 days have passed since calling the function, the funds would be stuck in the Silo contract regardless of the others users who can withdraw instantly.

& for

It's like a fixed-term bank deposit in which the term is 30-day.

If that's the intention then also the user who has initially called cooldownAssets()/cooldownShares(), can call these functions again to change their deposit time period & break the FIXED-TERM BANK DEPOSIT.

& for

Why should Zayn call the cooldownAssets() function again (for the second time) after he has redeemed his assets and started a cooldown period to claim his converted underlying asset?

This was because of the comment left by the judge

Alice can simply call cooldownAssets() again to set the cooldown time to the shorter 5 days. The staker can navigate away from the "unfair scenario" by cooling down once again.

@Shubh0412 is correct. The ensureCooldownOn modifier makes it so that stakers that previously cooled down while there was a non-zero cooldown time will have to wait unfairly for their cooldowns to end.

While other comments have explained that the stakers have agreed to the terms of the cooldown, it does not outweigh the unfairness of not being able to unstake immediately. The cooldown period may be set to 0 in the case of a known vulnerability, and the protocol will urge users to unstake their funds. If they are blocked from doing so because of this oversight, that will result in frozen funds, and at worst, loss of funds.

This previously downgraded issue has been upgraded by fatherGoose1

The primary issue with this implementation is regarding stakers who have called cooldown...() while a non-zero cooldownDuration was set, and then the cooldownDuration is set to 0, resulting a revert upon calling unstake(). While reading through this report and its duplicates, there is a clear division between reports that highlight this issue and those that don't.

I am in the process of de-duping these reports and assigning the proper severity levels. Reports that highlight this case will receive "Medium" while those that don't will be marked as QA.

Reports that do not highlight the reversion due to the ensureCooldownOn modifier have not accurately explained how the current implementation can cause significant impact.

fatherGoose1 changed the severity to QA (Quality Assurance)

fatherGoose1 marked the issue as grade-b

fatherGoose1 marked the issue as not selected for report