AmmGauge stake allows for reentrancy that can lead to stealing the contract balance

[code423n4]

Lines of code

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

Vulnerability details

Impact

Some ERC20 do allow for user's control of execution. For example, ERC777 has tokensReceived() hook. This way, an ability to reenter can be executed with the usage of any such tokens.

AmmGauge stake do not control for reentrancy and uses balance difference to calculate how much to add to user's accounted funds.

Suppose Bob has 100 tokens, runs stake and reenters 4 times, so run stake 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 Vault records 100 + 80 + 60 + 40 + 20 = 300 tokens for the 20 * 5 = 100 tokens Bob deposited.

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

Setting severity to high as that's the loss of funds of all other stakers.

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-04-backd/blob/main/backd/contracts/tokenomics/AmmGauge.sol#L103-L113

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

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/tokenomics/AmmGauge.sol#L131

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

Recommended Mitigation Steps

Introduce reentrancy control, for example a well-tested nonReentrant modifier.

[chase-manning]

Tokenomics contracts are out of scope

[gzeoneth]

Invalid due to out-of-scope. https://github.com/code-423n4/2022-04-backd/blob/main/README.md