Description

The withdraw function in DefinitiveStakingManager begins as follows:

/**
 * @dev Withdraw tokens from the strategy
 */
function withdraw(uint256 _lpTokenAmount, uint8 _index) public {
  StrategyStaker storage staker = strategyStakers[msg.sender];
  require(_lpTokenAmount > 0, "Withdraw amount can't be zero");
  require(_index < underlyingTokenAddresses.length, "Invalid index");
  
  // Withdraw all if amount is greater than staked
  if (_lpTokenAmount > staker.amount) {
    _lpTokenAmount = staker.amount;
  }

Since staker.amount may be zero, the rest of this function may continue with no tokens withdrawn despite the check on _lpTokenAmount.

Impact

The withdrawal process may be confusing to users with zero balance.

Further, suppose a user has zero balance in the staking manager, but nonzero balance in reward tokens. In order to redeem those tokens, they would have to call withdraw with nonzero _lpTokenAmount despite their balance being zero; otherwise, the function would revert. This is unintuitive.

Recommendations

We recommend performing the check on the amount being withdrawn. That is, after _lpTokenAmount is compared against the user balance and added to the rewardTokenBalance.

More discussion of withdraw's behavior concerning reward token balance is included in . If the suggestion to relocate token redemption logic is taken, then we simply recommend moving the _lpTokenAmount > 0 check to after it is compared against staker.amount.

Remediation

This issue has been acknowledged by Rainmaker, and a fix was implemented in commit 4b8af543↗.