GalloDaSballo
commented
2 years ago 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
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
contracts/BkdLocker.sol
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
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