search

code-423n4 / 2022-05-backd-findings

0 stars 0 forks source link

Users can unstake more AMM Token can their balance #139

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The unstakeFor() function declares and defines a local variable newBal after the transfer of AMM Tokens, and then continues with calculating the number of unstaked tokens to adjust user's balance. By re-entering the function from line 131 - IERC20(ammToken).safeTransfer(dst, amount); , the user can send more AMM tokens from the contract multiple times before the newBal and unstaked is calculated.

Proof of Concept

  1. Assume Alice has 2 EOA accounts. The contract currently has 500 AMM tokens, with Alice's Account A having 100 of them
  2. Using Account A, Alice calls unstakeFor() with amount 100 and Account B is dst address.
  3. Old balance on first call is 500, Alice is transferred 100.
  4. But Alice reenters unstakeFor() just after the transfer. This means the contract's balance is now 400 and Account's balance is still 100, while Account B now has 100.
  5. Alice reenters until contract's AMM tokens reaches 0. Now Account B will have 500 tokens.

Tools Used

Manual review

Recommended Mitigation Steps

Ensure to apply the Checks-Effects-Interactions pattern or some Reentrancy Guards

chase-manning commented 2 years ago

We don't support tokens that offer a reentrancy opportunity

GalloDaSballo commented 2 years ago

Dup of #19