AmmGauge stakeFor/unstakeFor allow for reentrancy that can lead to stealing the whole contract balance

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L108-L113
https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L130-L135

Vulnerability details

Some ERC20 do allow for user's control of transfer call execution. For example, ERC777 has tokensReceived() hook. This way, the execution flow can be reentered with the usage of any such tokens. This possibility can be a part of current implementation or can be introduced in the future with token contract upgrade. AmmGauge deals with an arbitrary AMM LP tokens, which can have such control now or be upgraded to have it in the future for various reasons.

AmmGauge's stakeFor() and unstakeFor() do not control for reentrancy and interact with AMM token contract before performing the accounting update. Balance difference is used to calculate how much to add to user's accounted funds. Together this allows for the following reentrancy based attack surface.

Suppose Bob has 100 tokens, runs stakeFor() and reenters 4 times, so run stakeFor() 5 times in total, with 20 token each time. His first run will recognize 100 tokens as a deposit, the second run will recognize 80, and only his last one will recognize only 20 tokens he actually sent. This way the AmmGauge records 100 + 80 + 60 + 40 + 20 = 300 tokens for the 20 * 5 = 100 tokens Bob actually deposited.

This way Bob will bloat his account and finally exit with all LP token funds AmmGauge have.

Setting severity to medium as that's the loss of funds of all other stakers conditional on AMM token execution flow control probability, which can be reasonably low, but isn't a zero.

Proof of Concept

stakeFor() allows reentrancy and determine the amount provided after pulling the funds from the user based on balance change:

https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L108-L113

Similarly, unstakeFor() allows reentrancy and can be gamed by orchestrating the cumulative balance change:

https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmGauge.sol#L130-L135

_userCheckpoint() called in the both cases will record zero Backd reward balance change on the nested runs, not preventing execution.

Recommended Mitigation Steps

Consider rearranging the operations so that external contract calls be placed after internal accounting is updated, for example:

Now (unstakeFor):

  uint256 oldBal = IERC20(ammToken).balanceOf(address(this));
  IERC20(ammToken).safeTransfer(dst, amount);
  uint256 newBal = IERC20(ammToken).balanceOf(address(this));
  uint256 unstaked = oldBal - newBal;
  balances[msg.sender] -= unstaked;
  totalStaked -= unstaked;

Rearranged (accounting first, interaction, correction):

  balances[msg.sender] -= amount;
  totalStaked -= amount;
  uint256 oldBal = IERC20(ammToken).balanceOf(address(this));
  IERC20(ammToken).safeTransfer(dst, amount);
  uint256 newBal = IERC20(ammToken).balanceOf(address(this));
  int256 unstakedDiff = (oldBal - newBal) - amount;
  if (unstakedDiff != 0) {
  		balances[msg.sender] -= unstakedDiff;
  		totalStaked -= unstakedDiff;
  		// place any additional logic for amount diff case here, if needed
  }

Also consider introducing reentrancy control, for example a nonReentrant modifier, to all user facing functions that perform fund transfers.

This also is relevant for AmmConvexGauge's stakeFor() and unstakeFor():

https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/AmmConvexGauge.sol#L157-L182

[chase-manning]

We do not support tokens that offer reentrancy opportunities

[GalloDaSballo]

Dup of #19