Function unstakeFor() and stakeFor() in AmmGauge is vulnerable to reentrancy attacfk

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/AmmGauge.sol#L103-L116 https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/AmmGauge.sol#L124-L138

Vulnerability details

Impact

contract AmmGauge don't protect itself against reentrancy attack with lock mechanism and in the middle of some logics in unstakeFor() and stakeFor() about contract token balances it makes some external calls to ammToken, if those calls somehow make another calls to attacker controlled addresses then attacker can reenter AmmGauge with stakeFor and change contract balances and then contract will increase attacker balances in contract multiple times for one deposit.

Proof of Concept

This is stakeFor code in AmmGauge:

    function stakeFor(address account, uint256 amount) public virtual override returns (bool) {
        require(amount > 0, Error.INVALID_AMOUNT);

        _userCheckpoint(account);

        uint256 oldBal = IERC20(ammToken).balanceOf(address(this));
        IERC20(ammToken).safeTransferFrom(msg.sender, address(this), amount);
        uint256 newBal = IERC20(ammToken).balanceOf(address(this));
        uint256 staked = newBal - oldBal;
        balances[account] += staked;
        totalStaked += staked;
        emit AmmStaked(account, ammToken, amount);
        return true;
    }

As you can see it store the value of IERC20(ammToken).balanceOf(address(this)); in oldBal and then tries to transferees user ammTokens but if that external call make some calls to attacker controlled address then attacker can reenter this contract by calling stakeFor() and stake some amount for himself and increase contract ammToken balance. then execution will return to original call and contract will check new balance of ammToken in line: uint256 newBal = IERC20(ammToken).balanceOf(address(this)); and because balance of contract increase because of multiple deposits then it's gonna count those deposits(which contract increase balance for them) as new deposits and increase attacker balance higher that it supposed to. these are the steps:

1. attacker calls stakeFor()
2. AmmGauge stores the balance of its address in ammToken and calls ammToken.transferFrom(attacker, AmmGauge address, amount)
3. ammToken is not ordinary ERC20 token and it transfers tokens is may call attacker addresses.
4. attacker call stakeFor() with bigAmount and normal execution will happen and balance of attacker in AmmGauge will increase by bigAmount
5. stakeFor code will then try to get new balances of AmmGauge in ammToken and then subtract oldBal from it to calculated deposited amount, but because new balance is amount + bigAmount is higher than oldBal then code will increase attacker balance by amount + bigAmount and code is counting bigAmount deposit multiple times.

the steps for unStakeFor() is similar. This is simple reentrancy attack and attacker can easily drain contract funds and the only requirments is that ammToken makes some external call to _from address

Tools Used

VIM

Recommended Mitigation Steps

add lock mechanism to contract

[chase-manning]

The AMM Tokens we use do not present a reentrancy opportunity.

[GalloDaSballo]

Dup of #19