Assessment reports>Staking>Medium findings>Cancellation of ,isDepositToken, still allows rewards to be claimed
Category: Coding Mistakes

Cancellation of isDepositToken still allows rewards to be claimed

Medium Severity
Medium Impact
Low Likelihood

Description

The isDepositToken mapping is used to validate whether a token is whitelisted to be staked in the contract.

function _stake(address _fundingAccount, address _account, address _depositToken, uint256 _amount) internal virtual {
    // ...
    require(isDepositToken[_depositToken], "RewardTracker: invalid _depositToken");

    IERC20(_depositToken).safeTransferFrom(_fundingAccount, address(this), _amount);

    // ... 
}

A similar check is performed upon unstaking of tokens.

function _unstake(address _account, address _depositToken, uint256 _amount, address _receiver) internal virtual {
    // ...
    require(isDepositToken[_depositToken], "RewardTracker: invalid _depositToken");

    // ...

    _burn(_account, _amount);
    IERC20(_depositToken).safeTransfer(_receiver, _amount);
}

Thus, if the isDepositToken mapping is set to False after previously being True, any amount of tokens that have been staked in the contract will not be able to be unstaked. Despite this, the rewards that have been accumulated will still be claimable.

Impact

The impact of this issue depends on the implementation of the rest of the protocol and several other considerations.

Since theoretically the isDepositToken can be called again to re-whitelist the token, the impact is diminished. However, in the case that the isDepositToken is a token that has been compromised and is no longer wanted by the system, this quick fix is no longer a viable alternative, and the issue becomes a severe problem, as the rewards are still accruing.

Recommendations

We recommend reconsidering the accrual of rewards for tokens that have been removed from the isDepositToken mapping. Essentially, they should not be considered towards the total amount of rewards that are claimable.

Remediation

This issue has been acknowledged by GammaSwap, and a fix was implemented in commit d29a27bb.

It is important to note, however, that the fix simply removes the isDepositToken check from the _unstake function. This could pose a security risk down the line if the depositBalances mapping is not properly updated on its own, as the _depositToken parameter is not checked for validity.

Zellic © 2024Back to top ↑