QA Report

[code423n4]

Report

• Low
  • L-01: use two-step process for critical address changes
• Non-Critical
  • N-01: emit an event when changing the configuration of a contract
  • N-02: AmmGauge doesn't use correct value for staking/unstaking events

Low

L-01: use two-step process for critical address changes

Consider using a two-step process for transferring the ownership of a contract. While it costs a little more gas, it's safer than transferring directly.

Here's an example from the Compound Timelock contract: https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol#L45-L58

Relevant code sections:

• https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L68

Non-Critical

N-01: emit an event when changing the configuration of a contract

There are multiple configuration functions that don't emit an event.

Relevant code:

• https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L68
• https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L74
• https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/VestedEscrow.sol#L74
• https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/InflationManager.sol#L58
• https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/Controller.sol#L33

There're probably a couple more that I missed

N-02: AmmGauge doesn't use correct value for staking/unstaking events

The functions verify the number of tokens that were transferred. The value is used to keep track of the internal balances. But, it isn't used for the event. There you use the user specified amount parameter:

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;
    
    // should be `staked` and not `amount`
    emit AmmStaked(account, ammToken, amount);
    return true;
}

Relevant code:

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

[GalloDaSballo]

## L-01: use two-step process for critical address changes
Personally disagree, but finding is valid

N-01: emit an event when changing the configuration of a contract

Agree

N-02: AmmGauge doesn't use correct value for staking/unstaking events

Finding is valid, nice find