Cancellation of isDepositToken
still allows rewards to be claimed
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.