QA Report

[code423n4]

• 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.

• Function setRewardsDistribution has a misleading revert message: "... changing the duration ..."

• A small loss in precision due to multiplication and division:

  IERC20(reward.token).transfer(treasury, d_reward / 5);
  d_reward = (d_reward * 4) / 5;

  A more accurate approach would be something like this:

  uint toTreasury = d_reward / 5;
  IERC20(reward.token).transfer(treasury, toTreasury);
  d_reward -= toTreasury;

• Consider introducing a reasonable upper limit for the rewards[_pid] array in ConvexStakingWrapper, otherwise if it grows too large it may exceed the gas limit when performing the _checkpoint and there is no way to remove it once added.

• ConvexStakingWrapper function addRewards fetches extraRewards and adds them to the list of rewards, but please note that extra rewards can change: https://github.com/convex-eth/platform/blob/main/contracts/contracts/BaseRewardPool.sol#L109-L119 Currently, ConvexStakingWrapper has no function to (sync) delete extra rewards. Consider implementing it.

• You should use safe casts here:

  deposits[_pid][msg.sender].amount += uint192(_amount);
  deposits[_pid][msg.sender].amount -= uint192(_amount);
  amount : uint192(_amount)

  Otherwise, if token amounts are exceeding these limits (e.g. rebasing tokens) the accounted and transferred amounts will differ. For instance, in function deposit it will add less to the user's balance but charge the full amount:

  deposits[_pid][msg.sender].amount += uint192(_amount);
  ...
  lpToken.safeTransferFrom(msg.sender, address(this), _amount);

• I don't think this check in function provide of contract USDMPegRecovery is correct:

  require(usdm.balanceOf(address(this)) >= totalLiquidity.usdm, "<liquidity");

  After you provide the liquidity (usdm3crv.add_liquidity), the balance of usdm will decrease, but totalLiquidity.usdm, will not, so the next time it will need to increase even more to reach this condition again. But not sure what was the exact intention here, so submitting this as of low severity FYI.

• Consider introducing a withdrawal deadline to indicate when it is too late and the user has to requestWithdraw again.

• I think the comment and the actual code is misleading here:

  if (extraToken == cvx) {
      //no-op for cvx, crv rewards
      rewards[_pid][CVX_INDEX].pool = extraPool;
  } 

[GalloDaSballo]

Function recoverERC20 in StakingRewards allows an owner to transfer out any token except stakingToken. I see 2 problems with this: Dup of #69 (med)

A small loss in precision due to multiplication and division: Agree

Consider introducing a reasonable upper limit for the rewards[_pid] array in ConvexStakingWrapper, otherwise if it grows too large it may exceed the gas limit when performing the _checkpoint and there is no way to remove it once added. Valid but Low probability

ConvexStakingWrapper function addRewards fetches extraRewards and adds them to the list of rewards, but please note that extra rewards can change: Great find

You should use safe casts here: Great fix to the issue of overpaying Also, dup of #194 (Med)

I don't think this check in function provide of contract USDMPegRecovery is correct: in lack of POC cannot but downgrade to non-critical, ultimately it seems to just ensure the function is only called when there's a ton of liquid token and not much deposited

Consider introducing a withdrawal deadline to indicate when it is too late and the user has to requestWithdraw again. Non-critical

I think the comment and the actual code is misleading here: Non-critical

Pretty interesting report that suffers from mediocre formatting as well as lack of links making the findings a lot less actionable

[GalloDaSballo]

3++

[GalloDaSballo]

Bumped to 4- to be third best submission

[JeeberC4]

@CloudEllie please create 2 new issue for the Med findings above.

[CloudEllie]

I've created separate issues for the following:

• Function recoverERC20 in StakingRewards allows an owner to transfer out any token except stakingToken #278
• You should use safe casts here #279