QA Report

[code423n4]

AddressProvider.sol

• L93/117 - It is validated that pool != address(0) but actually pool is needed to put it inside the ILiquidityPool interface, therefore, the best thing would be to request the interface directly in the signature, like this: addPool(ILiquidityPool pool) and removePool(ILiquidityPool pool).
  In this way, it would be avoided to validate that pool != address(0), since address(0) does not comply with the ILiquidityPool interface.

• L288 - An address is requested in the addStakerVault() function and as soon as the function starts it is put inside the IStakerVault interface, therefore, the best would be to request the interface directly in the signature, like this: addStakerVault(IStakerVault stakerVault).
  This way it would avoid starting to execute inside the function.

Controller.sol

• L33 - The setInflationManager() function performs two validations, the second would not be necessary if an IInflationManager is requested directly in the signature (as is done in the constructor).
  This would also have the benefit of not being wrapear on line 36.

• L39 - The addStakerVault() function has as its first validation, return false if this validation is true (!addressProvider.addStakerVault(stakerVault)),
  The problem with this validation is that in the implementation of AddressProvider it never returns false, therefore the validation is not necessary (it is also immutable, therefore it can only be modified in the deploy).

• L121/123/127/129 - The code of the function getTotalEthRequiredForGas() would be much cleaner if the signature contains the creation of the variable (returns (uint256 totalEthRequired)),
  in this way the creation of the variable in line 123 and the final return would be avoided.

contracts/zaps/PoolMigrationZap.sol

• L24/25 - If when executing: newPool_.getUnderlying(), we get address(0) as response, it should not be correct to set underlyingNewPools[address(0)] and this is currently happening. The validation of line 26: if (underlying == address(0)) continue; it should be earlier.

contracts/BkdLocker.sol

• L188 - It is not validated in the getShareOfTotalBoostedBalance() function that when the division is performed with totalLockedBoosted, totalLockedBoosted is != 0.

contracts/tokenomics/FeeBurner.sol

• L25 - The WETH address is hardcoded, this implies that this code is only usable in a single network. If it's on testnet, it can't be deployed on mainnet.
  If it's on mainnet, you can't test it.

• L79 - When the targetUnderlying_ variable is created, it is never validated that it is != address(0), this is important, since otherwise when swapAll() is executed they would be burning WETH.

contracts/tokenomics/KeeperGauge.sol

• L77 - The function requests an extra variable that is not requested. There is a comment that says to add it so that the compiler don't throw warnings.

contracts/StakerVault.sol

• L156/157 - First it should be validated that the src has an amount to transfer and then check if it needs allowance or not.

• L185 - If a malicious address is approved and if the src wants to change the approve it has, the spender could front run it to spend that approve you have and end up with more allowance.

contracts/tokenomics/InflationManager.sol

• L80/81 - In the mint() function of the minter contract it can only be executed by the deployer of the BkdToken contract.
  Therefore, there is not much benefit in mintRewards() being executed for any address that returns true in the
  modifieronlyGauge().

[GalloDaSballo]

L93/117 - It is validated that pool != address(0) but actually pool is needed to put it inside the ILiquidityPool interface,

I don't believe the finding to be valid, unless the Address library by OZ (which btw is not used in this contract) is doing the check, then the warden advice will actually accept address(0) as valid.

You will get a revert at line 101, which may or may not be what you would want in case of a 0 address

L288 - An address is requested in the addStakerVault() function

Arguably useful as you'd still get a revert when calling getToken

L33 - The setInflationManager()

I believe the warden is under the assumption that using an Interface as the type will perform a zero check, but that is not the case per the demo below.

contract GasTest is DSTest {
    GasTestStorageInMappingGood c0;
    GasTestStorageInMappingBad c1;

    function setUp() public {
        c0 = new GasTestStorageInMappingGood();
    }

    function testGas() public {
       c0.doSmth(IStuff(0x0000000000000000000000000000000000000000));
        
    }
}

interface IStuff {
    function doStuff() external view;
}
contract GasTestStorageInMappingGood {
    IStuff public storedThing;

    function doSmth(IStuff theThing) external{
        storedThing = theThing;
    }
}

I don't believe the findings related to casting to interface are valid as they seem to be based on invalid assumptions as well as a lack of code

L39 - The addStakerVault()

Agree, there will never be a false return there, only reverts, the check can be removed

L121/123/127/129 - The code of the function getTotalEthRequiredForGas()

Honestly really not a big deal and subjective as some people prefer explicit return and it's the same for the compiler

L24/25 - If when executing: newPool_.getUnderlying()

Agree that an early return makes sense

L188 - It is not validated in the getShareOfTotalBoostedBalance()

I don't believe this to be an issue nor necessary as Solidity >= 0.8.0 will do a division by zero check

L25 - The WETH address is hardcoded

While the recommendation to use immutable can be entertained, the rest of the claims are highly subject as inlining is always the cheapest way of storing data, and testing can be done on a fork-mainnet

L79 - When the targetUnderlying_ variable is created, it is never validated that it is != address(0),

I agree that the code may need refactoring, however the case of address(0) is handled inswap, personally I'd recommend the sponsor to consider refactoring the entire flow to offer less flexibility while giving stronger "good paths"
Because of the interesting food for thought, I think the finding is valuable

L77 - The function requests an extra variable that is not requested.

Considering that the code is not upgradeable I agree that the unused variable could just be refactored away

## L156/157 - First it should be validated that the src has an amount to transfer and then check if it needs allowance or not.
I'm not convinced as this boils down to opinion, you ultimately need to do both checks, so why prefer one over the other?

L185 - If a malicious address is approved and if the

Per the discussion on the standard, it's up to the user to avoid that, not the ERC programmer

L80/81 - In the mint()

The inflationManager keeps tracks of gauges and the token only let's the inflationManager mints, I think this finding needed further development to be actionable

[GalloDaSballo]

I'm conflicted on this report, the warden is trying and has original thoughts.
Some of the findings may come from invalid assumptions.
Lastly, the formatting could be improved by adding links to the files and line on each of the L80 making the report a lot more convenient to verify and take action on