sherlock-audit / 2024-06-makerdao-endgame-judging

5 stars 3 forks source link

EFCCWEB3 - Unrestricted Recovery of Rewards Tokens in Staking Contract #4

Closed sherlock-admin2 closed 3 months ago

sherlock-admin2 commented 3 months ago

EFCCWEB3

Medium

Unrestricted Recovery of Rewards Tokens in Staking Contract

Summary

The recoverERC20 function in the StakingRewards contract allows the owner to withdraw any ERC-20 tokens from the contract, except for the staking token. However, there is no check to prevent the owner from withdrawing the rewards tokens accumulated over time, potentially leading to a rug pull where the owner can drain the rewards tokens meant for users.

Vulnerability Detail

The recoverERC20 function is designed to allow the owner to recover any ERC-20 tokens accidentally sent to the contract, except for the staking token. However, there is no check to prevent the recovery of the rewards token, which accumulates over time in the contract to be distributed to stakers.

// Assume `rewardsToken` is the token used for rewarding stakers.
address rewardsToken = address(0x...); // Replace with actual rewards token address

// Function call by the owner to sweep rewards tokens
stakingRewardsContract.recoverERC20(rewardsToken, rewardsToken.balanceOf(address(stakingRewardsContract)));

In this scenario, the owner calls recoverERC20 with the rewardsToken address and the full balance of rewardsToken held by the contract. This transfers all accumulated rewards tokens to the owner's address, potentially "rugging" the depositors by taking away the rewards meant for them.

Impact

Users lose their accumulated rewards.

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/synthetix/StakingRewards.sol#L166

Code Snippet

  function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyOwner {
        require(tokenAddress != address(stakingToken), "Cannot withdraw the staking token");
        IERC20(tokenAddress).safeTransfer(owner, tokenAmount);
        emit Recovered(tokenAddress, tokenAmount);
    }

Tool used

Manual Review

Recommendation

function recoverERC20(address tokenAddress, uint256 tokenAmount)
    external
    onlyOwner
{
 ---      require(tokenAddress != address(stakingToken), "Cannot withdraw the staking token");
 +++   require(
        tokenAddress != address(stakingToken) && tokenAddress != address(rewardsToken),
        "Cannot withdraw the staking or rewards token"
    );
    IERC20(tokenAddress).safeTransfer(owner(), tokenAmount);
    emit Recovered(tokenAddress, tokenAmount);
}

Duplicate of #41

telome commented 3 months ago

The owner is assumed fully trusted and non malicious. From the competition rules: "Even when possible, governance is assumed to not confiscate/manipulate specific user funds/positions without a good reason. This means that reports that claim that governance can take specific users funds are not considered issues."