search

code-423n4 / 2022-02-concur-findings

2 stars 0 forks source link

Function `recoverERC20` in StakingRewards allows an owner to transfer out any token except `stakingToken` #278

Closed CloudEllie closed 2 years ago

CloudEllie commented 2 years ago

Judge @GalloDaSballo has assessed the 1st item in QA Report #254 as Medium risk. The relevant finding follows:

…

Function recoverERC20 in StakingRewards allows an owner to transfer out any token except stakingToken.

I see 2 problems with this:

  1. It should also forbid transferring of rewardsToken, otherwise an owner can drain the rewards and DDOS users withdrawals because there is no way to get back your stake tokens without claiming the rewards.
  2. It may be possible that someone accidentally sent stake tokens directly to the contract and these tokens will not be accounted in _totalSupply, thus it makes sense that an owner should be able to rescue these unaccounted tokens: stakingToken.balanceOf(address(this) - _totalSupply).

I assigned this issue a severity of low because I assume we can trust the owner not to exploit this :?

You should forbid recoverERC20 of rewardsToken, and may also allow transferring the surplus from _totalSupply of stakingToken. Usually, it is a good practice in such contracts to have an emergency withdrawal function, where users can get back their stake tokens but forfeit the rewards.

CloudEllie commented 2 years ago

Duplicate of #69