Backd Tokenomics contest - Ruhum's results

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/FeeBurner.sol#L43-L88 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/swappers/SwapperRouter.sol#L414-L425 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/swappers/SwapperRouter.sol#L439

Vulnerability details

Impact

While the SwapperRouter contract isn't explicitly in scope, it's a dependency of the FeeBurner contract which is in scope. So I think it's valid to make this submission.

The SwapperRouter contract uses the chainlink oracle to compute the minimum amount of tokens it should expect from the swap. The value is then used for the slippage check. But, if the chainlink oracle fails, for whatever reason, the contract uses 0 for the slippage check instead. Thus there's a scenario where swaps initiated by the FeeBurner contract can be sandwiched.

Proof of Concept

1. multiple swaps initiated through FeeBurner.burnToTarget()
2. SwapperRouter calls _minTokenAmountOut() to determine min_out parameter.
3. minTokenAmountOut() returns 0 when Chainlink oracle fails

Tools Used

none

Recommended Mitigation Steps

Either revert the transaction or initiate the transaction with a default slippage of 99%. In the case of Curve, you can get the expected amount through get_dy() and then multiply the value by 0.99. Use that as the min_out value and you don't have to worry about chainlink

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