Backd Tokenomics contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L104-L108

Vulnerability details

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L104-L108

    function startInflation() external override onlyGovernance {
        require(lastEvent == 0, "Inflation has already started.");
        lastEvent = block.timestamp;
        lastInflationDecay = block.timestamp;
    }

As lastEvent and lastInflationDecay are not initialized in the constructor(), they will remain to the default value of 0.

However, the permissionless executeInflationRateUpdate() method does not check the value of lastEvent and lastInflationDecay and used them directly.

As a result, if executeInflationRateUpdate() is called before startInflation():

1. L190, the check of if _INFLATION_DECAY_PERIOD has passed since lastInflationDecay will be true, and initialPeriodEnded will be set to true right away;
2. L188, since the lastEvent in totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent)); is 0, the totalAvailableToNow will be set to totalAvailableToNow ≈ currentTotalInflation * 52 years, which renders the constrains of totalAvailableToNow incorrect and useless.

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L115-L117

    function executeInflationRateUpdate() external override returns (bool) {
        return _executeInflationRateUpdate();
    }

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L187-L215

    function _executeInflationRateUpdate() internal returns (bool) {
        totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));
        lastEvent = block.timestamp;
        if (block.timestamp >= lastInflationDecay + _INFLATION_DECAY_PERIOD) {
            currentInflationAmountLp = currentInflationAmountLp.scaledMul(annualInflationDecayLp);
            if (initialPeriodEnded) {
                currentInflationAmountKeeper = currentInflationAmountKeeper.scaledMul(
                    annualInflationDecayKeeper
                );
                currentInflationAmountAmm = currentInflationAmountAmm.scaledMul(
                    annualInflationDecayAmm
                );
            } else {
                currentInflationAmountKeeper =
                    initialAnnualInflationRateKeeper /
                    _INFLATION_DECAY_PERIOD;

                currentInflationAmountAmm = initialAnnualInflationRateAmm / _INFLATION_DECAY_PERIOD;
                initialPeriodEnded = true;
            }
            currentTotalInflation =
                currentInflationAmountLp +
                currentInflationAmountKeeper +
                currentInflationAmountAmm;
            controller.inflationManager().checkpointAllGauges();
            lastInflationDecay = block.timestamp;
        }
        return true;
    }

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L50-L51

    // Used for final safety check to ensure inflation is not exceeded
    uint256 public totalAvailableToNow;

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L217-L227

    function _mint(address beneficiary, uint256 amount) internal returns (bool) {
        totalAvailableToNow += ((block.timestamp - lastEvent) * currentTotalInflation);
        uint256 newTotalMintedToNow = totalMintedToNow + amount;
        require(newTotalMintedToNow <= totalAvailableToNow, "Mintable amount exceeded");
        totalMintedToNow = newTotalMintedToNow;
        lastEvent = block.timestamp;
        token.mint(beneficiary, amount);
        _executeInflationRateUpdate();
        emit TokensMinted(beneficiary, amount);
        return true;
    }

Recommendation

Consider initializing lastEvent, lastInflationDecay in constructor().

or

Consider adding require(lastEvent != 0 && lastInflationDecay != 0, "...") to executeInflationRateUpdate().

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L90-L100

Vulnerability details

Every time the BkdLocker#depositFees() gets called, there will be a surge of rewards per locked token for the existing stakeholders.

This enables a well-known attack vector, in which the attacker will take a large portion of the shares before the surge, then claim the rewards and exit immediately.

While the _WITHDRAW_DELAY can be set longer to mitigate this issue in the current implementation, it is possible for the admin to configure it to a very short period of time or even 0.

In which case, the attack will be very practical and effectively steal the major part of the newly added rewards.

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/BkdLocker.sol#L90-L100

function depositFees(uint256 amount) external override {
    require(amount > 0, Error.INVALID_AMOUNT);
    require(totalLockedBoosted > 0, Error.NOT_ENOUGH_FUNDS);
    IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), amount);

    RewardTokenData storage curRewardTokenData = rewardTokenData[rewardToken];

    curRewardTokenData.feeIntegral += amount.scaledDiv(totalLockedBoosted);
    curRewardTokenData.feeBalance += amount;
    emit FeesDeposited(amount);
}

PoC

Given:

• Current totalLockedBoosted() is 100,000 govToken;
• Pending distribution fees amount is 1,000 rewardToken;

1. depositFees() is called to add 1,000 rewardToken;
2. The attacker frontrun the 1st transaction with a lock() transaction to deposit 100,000 govToken, taking 50% of the pool;
3. After the transaction in step 1 is mined, the attacker calls claimFees() and received 500 rewardToken.

As a result, the attacker has stolen half of the pending fees which belong to the old users.

Recommendation

Consider switching the reward to a rewardRate-based gradual release model, such as Synthetix's StakingRewards contract.

See: https://github.com/Synthetixio/synthetix/blob/develop/contracts/StakingRewards.sol#L113-L132

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L187-L215

Vulnerability details

When Minter.sol#_executeInflationRateUpdate() is called, if an _INFLATION_DECAY_PERIOD has past since lastInflationDecay, it will update the InflationRate for all of the gauges.

However, in the current implementation, the rates will be updated first, followed by the rewards being settled using the new rates on the gauges using inflationManager().checkpointAllGauges().

If the _INFLATION_DECAY_PERIOD has passed for a long time before Minter.sol#executeInflationRateUpdate() is called, the users may lose a significant amount of rewards.

On a side note, totalAvailableToNow is updated correctly.

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L187-L215

function _executeInflationRateUpdate() internal returns (bool) {
    totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));
    lastEvent = block.timestamp;
    if (block.timestamp >= lastInflationDecay + _INFLATION_DECAY_PERIOD) {
        currentInflationAmountLp = currentInflationAmountLp.scaledMul(annualInflationDecayLp);
        if (initialPeriodEnded) {
            currentInflationAmountKeeper = currentInflationAmountKeeper.scaledMul(
                annualInflationDecayKeeper
            );
            currentInflationAmountAmm = currentInflationAmountAmm.scaledMul(
                annualInflationDecayAmm
            );
        } else {
            currentInflationAmountKeeper =
                initialAnnualInflationRateKeeper /
                _INFLATION_DECAY_PERIOD;

            currentInflationAmountAmm = initialAnnualInflationRateAmm / _INFLATION_DECAY_PERIOD;
            initialPeriodEnded = true;
        }
        currentTotalInflation =
            currentInflationAmountLp +
            currentInflationAmountKeeper +
            currentInflationAmountAmm;
        controller.inflationManager().checkpointAllGauges();
        lastInflationDecay = block.timestamp;
    }
    return true;
}

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/InflationManager.sol#L110-L125

function checkpointAllGauges() external override returns (bool) {
    uint256 length = _keeperGauges.length();
    for (uint256 i; i < length; i = i.uncheckedInc()) {
        IKeeperGauge(_keeperGauges.valueAt(i)).poolCheckpoint();
    }
    address[] memory stakerVaults = addressProvider.allStakerVaults();
    for (uint256 i; i < stakerVaults.length; i = i.uncheckedInc()) {
        IStakerVault(stakerVaults[i]).poolCheckpoint();
    }

    length = _ammGauges.length();
    for (uint256 i; i < length; i = i.uncheckedInc()) {
        IAmmGauge(_ammGauges.valueAt(i)).poolCheckpoint();
    }
    return true;
}

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/KeeperGauge.sol#L110-L117

function poolCheckpoint() public override returns (bool) {
    if (killed) return false;
    uint256 timeElapsed = block.timestamp - uint256(lastUpdated);
    uint256 currentRate = IController(controller).inflationManager().getKeeperRateForPool(pool);
    perPeriodTotalInflation[epoch] += currentRate * timeElapsed;
    lastUpdated = uint48(block.timestamp);
    return true;
}

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/InflationManager.sol#L507-L519

function getKeeperRateForPool(address pool) external view override returns (uint256) {
    if (minter == address(0)) {
        return 0;
    }
    uint256 keeperInflationRate = Minter(minter).getKeeperInflationRate();
    // After deactivation of weight based dist, KeeperGauge handles the splitting
    if (weightBasedKeeperDistributionDeactivated) return keeperInflationRate;
    if (totalKeeperPoolWeight == 0) return 0;
    bytes32 key = _getKeeperGaugeKey(pool);
    uint256 poolInflationRate = (currentUInts256[key] * keeperInflationRate) /
        totalKeeperPoolWeight;
    return poolInflationRate;
}

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/Minter.sol#L173-L176

    function getKeeperInflationRate() external view override returns (uint256) {
        if (lastEvent == 0) return 0;
        return currentInflationAmountKeeper;
    }

PoC

Given:

• currentInflationAmountAmm: 12,000 Bkd (1000 per month)
• annualInflationDecayAmm: 50%
• initialPeriodEnded: true
• lastInflationDecay: 11 months ago
• _INFLATION_DECAY_PERIOD: 1 year

1. Alice deposited as the one and only staker in the AmmGauge pool;
2. 1 month later;
3. Minter.sol#_executeInflationRateUpdate() is called;
4. Alice claimableRewards() and received 500 Bkd tokens.

Expected Results:

• Alice to receive 1000 Bkd tokens as rewards.

Actual Results:

• Alice received 500 Bkd tokens as rewards.

Recommendation

Consider moving the call to checkpointAllGauges() to before the currentInflationAmountKeeper is updated.

function _executeInflationRateUpdate() internal returns (bool) {
    totalAvailableToNow += (currentTotalInflation * (block.timestamp - lastEvent));
    lastEvent = block.timestamp;
    if (block.timestamp >= lastInflationDecay + _INFLATION_DECAY_PERIOD) {
        controller.inflationManager().checkpointAllGauges();
        currentInflationAmountLp = currentInflationAmountLp.scaledMul(annualInflationDecayLp);
        if (initialPeriodEnded) {
            currentInflationAmountKeeper = currentInflationAmountKeeper.scaledMul(
                annualInflationDecayKeeper
            );
            currentInflationAmountAmm = currentInflationAmountAmm.scaledMul(
                annualInflationDecayAmm
            );
        } else {
            currentInflationAmountKeeper =
                initialAnnualInflationRateKeeper /
                _INFLATION_DECAY_PERIOD;

            currentInflationAmountAmm = initialAnnualInflationRateAmm / _INFLATION_DECAY_PERIOD;
            initialPeriodEnded = true;
        }
        currentTotalInflation =
            currentInflationAmountLp +
            currentInflationAmountKeeper +
            currentInflationAmountAmm;
        lastInflationDecay = block.timestamp;
    }
    return true;
}

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/AmmGauge.sol#L108-L110

Vulnerability details

Some ERC20 compatible token implementations, such as ERC777-like tokens, allow a callback call after the balances are updated in transfer(). This callback could be used to re-enter the contract.

In AmmGauge.sol#unstakeFor(), unstaked is calculated by comparing the oldBal with current balance after the external call (SafeTransferLib.safeTransferFrom()). This allows the attacker to re-enter the contract and call stakeFor() to increase balances[account].

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/tokenomics/AmmGauge.sol#L108-L110

    uint256 oldBal = IERC20(ammToken).balanceOf(address(this));
    IERC20(ammToken).safeTransferFrom(msg.sender, address(this), amount);
    uint256 newBal = IERC20(ammToken).balanceOf(address(this));

PoC

Given:

• ammToken is XYZ;
• XYZ is a ERC777-like token;
• current XYZ.balanceOf(AmmGauge) is 1,000,000e18;
• current XYZ.balanceOf(attacker) is 0;
• current AmmGauge.balances[attacker] is 10,000e18.

The attacker can increase AmmGauge.balances[attacker] without paying for it.

• unstakeFor(attacker, attacker, 10,000e18)
  • L376 oldBal = IERC20(ammToken).balanceOf(address(this)) = 1,000,000e18
  • L382 IERC20(ammToken).safeTransfer(attacker, 10,000e18);
    • update balance of AmmGauge and attacker
      • XYZ.balanceOf(AmmGauge): 1,000,000e18 -> 990,000e18
      • XYZ.balanceOf(attacker): 0 -> 10,000e18
      • see https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.6.0/contracts/token/ERC777/ERC777.sol#L377
    • In the hook of to of the transfer() (eg, the attacker's address), re-enter and call AmmGauge's AmmGauge.stakeFor(attacker, 10,000e18)
      • L331 oldBal = IERC20(ammToken).balanceOf(address(this)) = 990,000e18
      • L338 IERC20(ammToken).safeTransferFrom(attacker, address(this), 10,000e18);
        • XYZ.balanceOf(AmmGauge): 990,000e18 -> 1,000,000e18
        • XYZ.balanceOf(attacker): 10,000e18 -> 0
      • L339 staked = IERC20(ammToken).balanceOf(address(this)) - oldBal = 1,000,000e18 - 990,000e18 = 10,000e18
      • L341 balances[attacker] += 10,000e18;
        • balances[attacker]: 10,000e18 -> 20,000e18
    • IERC20(ammToken).safeTransfer(attacker, 10,000e18); (end of the call)
  • L384 unstaked = oldBal - IERC20(ammToken).balanceOf(address(this)) = 1,000,000e18 - 1,000,000e18 = 0
  • L390 balances[src] -= 0: balances[attacker] remain unchanged: 20,000e18

As a result:

• XYZ.balanceOf(AmmGauge): 1,000,000e18 -> 1,000,000e18
• XYZ.balanceOf(attacker): 0 -> 0
• AmmGauge.balances[attacker]: 10,000e18 -> 20,000e18

Now, the attacker can call unstakeFor(attacker, attacker, 20,000e18) and withdraw 20,000e18 XYZ

The attacker can repeat the steps above to inflate AmmGauge.balances[attacker] to XYZ.balanceOf(AmmGauge) = 1,000,000e18, and call unstakeFor(attacker, attacker, 1,000,000e18) to empty all the XYZ tokens in AmmGauge.

Recommendation

Consider adding nonReentrant modifier to unstakeFor() and stakeFor().