Backd Tokenomics contest - berndartmueller's results

Lines of code

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

Vulnerability details

Impact

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

Proof of Concept

The FeeBurner.burnToTarget function will try to swap more of an ERC20 token than the contract actually received and due to transfering the token into the SwapperRouter contract, the token balance is insufficient and the transfer will revert.

tokenomics/FeeBurner.sol#L70

token_.safeTransferFrom(msg.sender, address(this), tokenBalance_); // @audit-info less tokens will be received in the contract when using fee-on transfer tokens
if (address(token_) == targetUnderlying_) continue;
_approve(address(token_), address(swapperRouter_));
swapperRouter_.swap(address(token_), _WETH, tokenBalance_); // @audit-info the swap function transfers the `token_` to itself and due to `tokenBalance_` not reflecting the correct token amount in the contract, the swap will revert

Tools Used

Manual review

Recommended mitigation steps

As other contracts (e.g. AmmGauge.stakeFor) already handle fee-on transfer tokens correctly, make sure also FeeBurner.burnToTarget does so.

Compare the token balance before the transfer and after the transfer and use the delta as the actual swap amount to prevent the FeeBurner.burnToTarget function reverting for fee-on transfer tokens:

function burnToTarget(address[] memory tokens_, address targetLpToken_)
    public
    payable
    override
    returns (uint256 received)
{
    ...

    uint256 oldBal = token_.balanceOf(address(this));
    token_.safeTransferFrom(msg.sender, address(this), tokenBalance_);
    uint256 swapAmount = token_.balanceOf(address(this)) - oldBal;

    if (address(token_) == targetUnderlying_) continue;
    _approve(address(token_), address(swapperRouter_));
    swapperRouter_.swap(address(token_), _WETH, swapAmount); // @audit-info use `swapAmount`

    ...
}

[G-01] Unnecessary check for positive value

Description

A uint256 value can not be negative, hence there is no need to check for it.

Findings

tokenomics/InflationManager.sol#L589

totalLpPoolWeight = totalLpPoolWeight > 0 ? totalLpPoolWeight : 0;

tokenomics/InflationManager.sol#L602

totalAmmTokenWeight = totalAmmTokenWeight > 0 ? totalAmmTokenWeight : 0;

tokenomics/InflationManager.sol#L575

totalKeeperPoolWeight = totalKeeperPoolWeight > 0 ? totalKeeperPoolWeight : 0;

Recommended mitigation steps

Remove the check and use the value directly to save gas.

[G-02] Unnecessary poolCheckpoint function call

The AmmGauge and KeeperGauge contracts call the function poolCheckpoint() within the kill() function. Therefore, functions which call this kill() function do not have to additionally call the poolCheckpoint() function.

Description

A uint256 value can not be negative, hence there is no need to check for it.

Findings

tokenomics/InflationManager.sol#L427
tokenomics/InflationManager.sol#L461

Recommended mitigation steps

Remove the call to poolCheckpoint() to save gas.