xiaoming90 - `rescueTokens` feature is broken

[sherlock-admin2]

xiaoming90

Medium

rescueTokens feature is broken

Summary

The rescue function is broken, and tokens cannot be rescued when needed, leading to assets being stuck in the contract.

Vulnerability Detail

The ClonedCoolDownHolder contains a feature that allows the protocol to recover lost tokens, as per the comment in Line 22 below. This function is guarded by the onlyVault modifier. Thus, only the vault can call this function.

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/ClonedCoolDownHolder.sol#L23

File: ClonedCoolDownHolder.sol
22:     /// @notice If anything ever goes wrong, allows the vault to recover lost tokens.
23:     function rescueTokens(IERC20 token, address receiver, uint256 amount) external onlyVault {
24:        token.checkTransfer(receiver, amount);
25:     }
26: 

However, it was found that none of the vaults could call the rescueTokens function. Thus, this feature is broken.

Impact

Medium. The rescue function is broken, and tokens cannot be rescued when needed, leading to assets being stuck in the contract.

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/ClonedCoolDownHolder.sol#L23

Tool used

Manual Review

Recommendation

Consider allowing the protocol admin to call the rescueTokens function directly, or update the implementation of vaults to allow the vault to call the rescueTokens function.

[sherlock-admin2]

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

0xmystery commented:

Lacks proof to substantiate the bug

[xiaoming9090]

Escalate.

This should be a valid issue. From the codebase, one can already see that the rescue function is broken. Since the rescue function is broken, if the protocol wants to rescue some tokens, it would not be able to, leading to a loss of assets.

[sherlock-admin3]

Escalate.

This should be a valid issue. From the codebase, one can already see that the rescue function is broken. Since the rescue function is broken, if the protocol wants to rescue some tokens, it would not be able to, leading to a loss of assets.

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.

[lemonmon1984]

This issue should be valid. Due to the lack of implementation, it's challenging to provide a proof of concept, but the issue is clear from the codebase. Additionally, GitHub issue #100 reports the same problem and should be duped into this one.

[mystery0x]

Escalate.

This should be a valid issue. From the codebase, one can already see that the rescue function is broken. Since the rescue function is broken, if the protocol wants to rescue some tokens, it would not be able to, leading to a loss of assets.

Will have the sponsors look into this finding. But it seems to me this function belongs to an abstract contract that's meant to be inherited by contracts needing to use it as determined by the protocol. For example, it's being inherited by Kelp.sol for whoever _vault to use it:

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/Kelp.sol#L52-L55

contract KelpCooldownHolder is ClonedCoolDownHolder {
    bool public triggered = false;

    constructor(address _vault) ClonedCoolDownHolder(_vault) { }

Additionally, finding of this nature is rated low given that it's optionally needed. Also, all vaults are deemed out of scope for this contest, and hence should be informational IMO.

[WangSecurity]

Also, all vaults are deemed out of scope for this contest

As I see not all vaults are OOS:

1. BaseStakingVault.sol
2. PendlePTEtherFiVault.sol
3. PendlePTKelpVault.sol
4. PendlePTStakedUSDeVault.sol

Hence, I would agree this report identifies an issue that breaks core contract functionality leading to a loss of funds. Planning to accept the escalation and validate with medium severity.

[WangSecurity]

Result: Medium Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted

[WangSecurity]

@mystery0x I see that 100 is a duplicate, are there other duplicates?

[mystery0x]

Lacks proof to substantiate the bug

Nope. #72 and #100 are the only two reports submitting this finding.

[lemonmon1984]

@mystery0x the labels on #100 hasn't been updated yet.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/notional-finance/leveraged-vaults/pull/100

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.