code423n4 commented 2 years ago

1. `AddressProvider.sol` does not have a `getSwapperRouter` function

Line References

[AddressProvider.sol]()

Impact

In FeeBurner.sol, the _swapperRouter function gets the swapper router from the address provider. The AddressProvider.sol and IAddressProvider.sol contracts do not seem to have a getSwapperRouter function. If the _addressProvider in FeeBurner.sol is the AddressProvider.sol contract or implements IAddressProvider.sol then FeeBurner.sol would not function correctly or at all.

Proof of concept

The _swapperRouter function is called in the burnToTarget function which should then revert thus making burnToTarget uncallable.

Recommended Mitigation Steps

Add the getSwapperRouter function in the AddressProvider.sol and IAddressProvider.sol contracts.

2. Incompatibility with deflationary/rebase/fee-on-transfer tokens in `FeeBurner.sol`

Line References

FeeBurner.sol#L85

Impact

In the burnToTarget function of FeeBurner.sol, if the tokens being transferred are tokens whose balances change during a transfer such as deflationary and fee-on-transfer tokens, then the swap could fail since the swap input amount might be larger than the actual amount of tokens received in FeeBurner.sol.

Recommended Mitigation Steps

When tokens are transferred to the contract, check the balance of the contract before and after the transfer. Use the difference in the balances as the input amount for the swap call.

3. Return value for `mint` is not checked in `InflationManager.sol`

Line References

InflationManager.sol#L81

Minter.sol#L126-L135

Impact

In Minter.sol, if inflation has not started and lastEvent == 0 then the mint function will return false. In InflationManager.sol, the mintRewards function does not check the return value for the mint call. When mintRewards is called by a gauge and fails, the transaction does not revert.

Proof of Concept

When calling claimRewards in AmmGauge.sol, the call to mintRewards does not revert if minting has failed and the user loses their rewards.

    function claimRewards(address beneficiary) external virtual override returns (uint256) {
        require(
            msg.sender == beneficiary || _roleManager().hasRole(Roles.GAUGE_ZAP, msg.sender),
            Error.UNAUTHORIZED_ACCESS
        );
        _userCheckpoint(beneficiary);
        uint256 amount = perUserShare[beneficiary];
        if (amount <= 0) return 0;
        perUserShare[beneficiary] = 0;
        controller.inflationManager().mintRewards(beneficiary, amount);
        emit RewardClaimed(beneficiary, amount);
        return amount;
    }

The KeeperGauge.sol and LpGauge.sol contracts would fail in a similar way in addition to any other gauges that call mintRewards.

Recommended Mitigation Steps

Consider either returning a bool value for the mintRewards function in InflationManager.sol and check the return value in the gauges or insert the mint call in a require,

require(Minter(minter).mint(beneficiary, amount))

GalloDaSballo commented 2 years ago