0xreadyplayer1 - Inactive Reward tokens can never be recovered : Funds will be stuck indefinitely

[sherlock-admin2]

0xreadyplayer1

high

Inactive Reward tokens can never be recovered : Funds will be stuck indefinitely

Summary

The Lock.sol holds an array of reward tokens that are used by the protocol to incentivize the stakers for locking their staking tokens. The rewards are standard ERC20 tokens. There is a method to add reward tokens to the lock system and that method is addReward which is owner only.

While most rewards will be active , some of them might be in-active for longer periods of time i.e 2 years or 4 years

In that time , there is a special function to recover those in-active reward tokens from the Lock system

and that method is recoverERC20 which is used to recover those in-active tokens

if and only if they are not active ( lastUpdatedTime= 0 ) and the token to be recovered is not the staking token.

However , there's a flaw in the code that assumes that when the token is in active for years , the lastUpdatedTime might be

equal to zero however , there's no place in the code that sets the token to inactive state ( marking its lastUpdatedTime= 0 )

All the points that manipulate the lastUpdatedTime , they always set it to

non-zero block.timestamp > 0 because block.timestamp can never be zero

Vulnerability Detail

The issue exists in Lock.recoverERC20 method

    /// @notice Allows the contract owner to recover ERC-20 tokens sent to the contract under specific conditions.
    /// @dev This function can only be executed by the contract owner and is meant for recovering tokens that are not actively being used as rewards or are not the staking token itself.
    ///      It checks if the token is currently active as a reward or if it's the staking token to prevent accidental or unauthorized recovery.
    /// @param tokenAddress The address of the ERC-20 token to be recovered.
    /// @param tokenAmount The amount of the token to be recovered.
    function recoverERC20(
        address tokenAddress,
        uint256 tokenAmount
    ) external onlyOwner {
        if (rewardData[tokenAddress].lastUpdateTime != 0) revert ActiveReward(); // Ensure the token is not currently active as a reward.
        if (tokenAddress == stakingToken) revert WrongRecoveryToken(); // Prevent recovery of the staking token.
        IERC20(tokenAddress).safeTransfer(owner(), tokenAmount); // Transfer the specified amount of the token to the contract owner.
        emit Recovered(tokenAddress, tokenAmount); // Emit an event to log the recovery operation.
    }

The Natspec states the purpose of this method :

meant for recovering tokens that are not actively being used as rewards or are not the staking token itself.

The method is not intended to allow recovering tokens that are active , ensured by following line

if (rewardData[tokenAddress].lastUpdateTime != 0) revert ActiveReward(); // Ensure the token is not currently active as a reward.

it means if lastUpdateTime is 0 then it will allow withdrawing it from Lock

however ,if we look at all the places where lastUpdateTime is being set

  function addReward(address _rewardToken) external override onlyOwner {
        //snip
        reward.lastUpdateTime = block.timestamp;
        //snip
    }

 function _notifyReward(address _rewardToken, uint256 reward) internal {
       //snip
        Reward storage r = rewardData[_rewardToken]; // Accesses the reward structure for the specified token.
        r.lastUpdateTime = block.timestamp; // Sets the last update time to now.
       //snip

    }

If we check these assignments , its evident that lastUpdateTime will always be >0 because block.timestamp is always >0

As lastUpdateTime will never be set to 0 , reward tokens will never be taken out from Lock contract

and tokens worth Billions of Dollars will get stuck in the contract that are not active.

PoC

Insert this into a new file inside tests ExploitTest.t.sol

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.23;

import "forge-std/Test.sol";
import "./Setup.sol";
import  "./../src/interfaces/ILock.sol";
/// @title ExploitTest for Lock
contract ExploitTest is Test, Setup {

 function testDosRecoverInActiveReward() public{

    // ###### Simulate some activity in lock contract
   // Simulating deposits by three different users
        vm.prank(user1);
        lock.stake(100e18, user1, 0);
        vm.prank(user2);
        lock.stake(100e18, user2, 1);
        vm.prank(user3);
        lock.stake(100e18, user3, 2);

        // Distributing rewards to the staking contract
        vm.prank(deployer);
        _rewardToken.transfer(address(lock), 600e18);

        vm.prank(deployer);
        lock.notifyUnseenReward(rewardTokens);

        // Checking that rewards are allocated correctly
        vm.prank(user1);
        lock.getAllRewards();
        assertEq(_rewardToken.balanceOf(user1), 100e18);
        vm.prank(user2);
        lock.getAllRewards();
        assertEq(_rewardToken.balanceOf(user2), 200e18);
        vm.prank(user3);
        lock.getAllRewards();
        assertEq(_rewardToken.balanceOf(user3), 300e18);

        vm.prank(deployer);

        _rewardToken.transfer(address(lock), 600e18);

    // ###### Try to recover in-active funds as a deployer

    vm.startPrank(deployer);

    address token = lock.rewardTokens(0);
    (uint256 lastUpdateTime,,)= lock.rewardData(token);
    console.log("last update time is ",lastUpdateTime);
    // let's advance time to 4 years with no activity
    vm.warp(block.timestamp+ 365*4 days);

    // expect revert due to incorrect active reward error 
    lock.recoverERC20(address(_rewardToken), _rewardToken.balanceOf(address(lock)));

 }

}

PoC output

Impact

Loss of funds to the protocol when reward tokens are not used as active rewards and tokens will be stuck.

Severity

As this issue causes Definite loss of funds without (extensive) limitations of external conditions, according to sherlock rules , it's a valid High severity issue.

Code Snippet

https://github.com/sherlock-audit/2024-05-gamma-staking/blob/main/StakingV2/src/Lock.sol#L659

Tool used

Manual Review

Recommendation

Apply some threshold for marking a token as in-active like if 365 days are passed , and the token is not used as a reward token

that token can be recovered.

Another method can be to add a setter method for marking rewards tokens to be in-active and then withdrawing through

recoverERC20 method.

[santipu03]

I believe the comments on that function are outdated. In the previous version of the codebase, there was a function to remove a reward token, but it was deleted before the Sherlock audit. Most probably that comment was referring to a reward token that was removed and could be recovered by the admins.

However, because now there's no way to remove reward tokens, those tokens cannot be recovered from the contract, and that is the intended design. It doesn't make any sense for the admins to recover some tokens from old rewards because maybe some users haven't claimed them yet so they'd lose them.

[0xreadyplayer1]

Hi @santipu03 we can not assume anything . As the code comments clearly states meant for recovering tokens that are not actively being used as rewards or are not the staking token itself , the code is indeed designed for what's in there and we should not assume that protocol will leave those tokens for other users . This functionality is intended for the time when the admin actually wants to recover tokens without adding any extra edge case here 'users have not claimed... etc' .

if this was the concern , the protocol MUST NOT even have added this method.

Also, There should be no regards to either codebase has something before the audit . Because this is the scope , and the attack path is clear , i believe it's a valid issue.

[santipu03]

That comment made sense when there was a function to remove reward tokens, and therefore to set the lastUpdateTime at zero. Now that the function is not there, no reward tokens can be recovered, this is the protocol design.

[0xreadyplayer1]

Hi @santipu03 I respect your intuitive answers but as the fellow whitehat said ,

When talking about the severity of an issue, we should not talk about how likely it is to happen (some almost impossible examples that actually happened: the Luna crash, or USDC depeg).

The Sherlock rules also does not talk about the possibility, since it is impossible to quantify. The scenario described above is possible (however less likely ) and falls under "certain external conditions or specific states".

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained.

The losses must exceed small, finite amount of funds, and any amount relevant based on the precision 

or significance of the loss.

Owing to above reasons , i believe the issue is valid and i look forward to what the head of the judging thinks.

[santipu03]

Your comment doesn't relate to this issue because in this case the scenario you describe will never be given. The two possible scenarios are the following:

1. If the admins add a reward token but they DO NOT SEND tokens to the contract, there's no need to call recoverERC20 because there's nothing to recover.
2. If the admins add a reward token and they DO SEND tokens to the contract, anyone can call notifyUnseenReward and those tokens will be distributed between all stakers, so it makes no sense for the admins to recover those tokens because they're getting distributed.

There are no possible scenarios where the admins add a reward token, send tokens, and then they need to recover them. For this reason, this issue is invalid.

[0xreadyplayer1]

After my unbiased review , i conclude that the decision is valid. Thank you @santipu03 for putting so much explanation , really helping me learn where to focus more on finding valid bugs. I appreciate it.