unstakeFor method in AmmConvexGauge.sol does not respect CEI standard.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/AmmConvexGauge.sol#L177

Vulnerability details

Issue: unstakeFor method does not respect CEI which can lead to reentrancy attack.

Consequences: If a token that has a hook on transfer is used, an attacker can use unstakeFor to drain the AmmConvexGauge of funds by calling unstakeFor via a custom contract that will reenter in the unstakeFor method once it receives the tokens on line 177.

File: AmmConvexGauge.sol
170:     function unstakeFor(address dst, uint256 amount) public virtual override returns (bool) {
171:         require(amount > 0, Error.INVALID_AMOUNT);
172:         require(balances[msg.sender] >= amount, Error.INSUFFICIENT_BALANCE);
173: 
174:         _userCheckpoint(msg.sender);
175: 
176:         crvRewardsContract.withdrawAndUnwrap(amount, false);
177:         IERC20(ammToken).safeTransfer(dst, amount);
178:         balances[msg.sender] -= amount;
179:         totalStaked -= amount;
180:         emit AmmUnstaked(msg.sender, ammToken, amount);
181:         return true;
182:     }

Proof of Concept

• AmmConvexGauge balance = 100 tokens.
• Attacker has 10 tokens staked.
• He can call unstakeFor as src == dst and reenter by external call on line 177 until the balance is 0.
• The checks and updates of the balance after the external call, will pass once the balance is 0, making the attacker to successfully drain the contract.

Mitigations

Consider adding the transfer of the tokens after the balances are updated, furthermore, consider adding a reentrancy guard.

[chase-manning]

We do not support tokens that offer reentrancy opportunities

[GalloDaSballo]

See #158