Backd Tokenomics contest - fatherOfBlocks's results

The finding at face value is valid and it's impact is a DOS.

A division by zero in keeperRecords[beneficiary].feesInPeriod[i].scaledDiv(perPeriodTotalFees[i]), will cause an instantaneous revert making it impossible to claim fees.

After more reading I believe that the finding is valid and can cause issues if there's one epoch without fees, if for any reason the protocol skips one epoch via advanceEpoch, while no perPeriodTotalFees are increased, then, because keeperRecords[beneficiary].nextEpochToClaim will include the empty epoch, the beneficiary will not be able to receive fees.

Given that setup is necessary for this to happen, I believe Medium Severity to be more appropriate

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

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

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

L27/46/112/113 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

Saves deployment cost which is hard to quantify, in lack of details cannot give it any points.

L64/71/98/102/176/185/199/241/242/260/270/295/296/325/428/434 - The require are too expensive, it would be saved using the Errors custom + if, instead of requires. Another option could be to use a private view function.

Same deal, no Code = no points

L101/102/103/118/119/121/122/123 - The get calls to other contracts, being a view method, do not generate any cost, therefore, it would be less expensive not to create variables and call the getter directly.

Doing a call to a view function generates a STATICCALL which costs 100 fixed gas plus the gas cost of the operations, this finding is invalid

L49/50/51/66/69/71 - The get calls to other contracts, being a view method, do not generate any cost, therefore, it would be less expensive not to create variables and call the getter directly.

Invalid per above

L66 - A variable is created and gas is consumed to fill a variable that will only be used if the if in line 68 is true, since it is only used within the if, it should be created within the if.

Agree, would save gas on the "bad path" hence hard to quantify

L31 - Gas can be saved if instead of a require an if is used, with the custom error already created.

Disagree in lack of POC

L7 - In the uncheckedInc() function, the gas could be further optimized, if instead of unchecked{a+1;} it was unchecked {++a;}

Should save 5 gas

L63 - It is less expensive to validate that variable != 0, than to validate variable > 0.

Saves 3 gas

L91/92/137/139/254/301 - It is less expensive to validate that variable != 0, than to validate variable > 0.

Only for requires, L91/92/137/139 4 * 3 = 12

L91/92/119/137/208 - Gas can be saved if instead of a require an if is used, with the custom error already created.

0 per above

L140/144 - --i is less expensive than i = i - 1;

Saves 5 gas 5 * 2 = 10

150/151 - A newTotal variable is created, to only be used in one place, it is not necessary and gas could be saved by not doing it.

3 gas

L117 - It is less expensive to validate that variable != 0, than to validate variable > 0.

Only for requires

L59 - The if that validates: (amount <= 0) does not make much sense, since in uint there is no < 0, therefore the correct validation is:

3 gas

L68/114 - The if that validates param > 0, could become less expensive if instead of >, a != is used.

Only for requires

L111/115 - A currentRate variable is created, which is only used in one place, therefore it is not necessary and more gas is spent.

3 gas

L101 - The claim() function appears as nonReentrant, but there is no reason for reentrant to occur, since only a transferFrom is performed

Arguable as Slither would flag the event for potential reEntrancy, will not award points here

L97/98 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

3

L103/104 - It is possible to create a variable in memory for revokedTime[msg.sender], in this way less gas would be used.

Saves 94 gas (100 - 3 - 3)

L52/53/54 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

See above

L54 - In the revoke() function the storage treasury address is used multiple times, one way to save gas is to create a variable in memory.

97 + 97 + 94 gas saved

L83/84 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

3

L89 - It is not necessary for the function to be nonReentrant, since the transfers that occur are only in the rewardToken address that is set

I'd keep the guard in lack of CEI pattern (and dodge the wrath of the spam c4 submissions)

L134/135/155/156/168/169 - It is not necessary to create a variable that will only be used once, it would save gas if it is used directly.

Saves 9 gas

L28/29/86/98/110/111 - The modifier generates a lot of gas cost, it would be less expensive to use an if with a custom error or a private view function.

It's an internal function ser

L59/98 - ++epoch uses less gas than epoch++;

5 gas

L39/78/82/126/140 - The requirements are very expensive, it could save gas if instead of a modifier it is an if with a custom error or a private view function.

See above

L140 - Gas is saved, if instead of totalClaimable > 0 you can use totalClaimable != 0.

3 (technically 6 but I already judged tons with 3)

L63 - It should be amount == 0, it doesn't make sense for amount to be <= 0, since it is uint256.

Saves 3 gas

L88/104/125/147 - Gas would be saved if instead of variable > 0 variable != 0 is used.

Only for requires, 2 * 3 = 6

Personally I think this type of report can be drastically improved by:

• Bulking the fix instead of grouping by file (humans think in problems and ideas, not in files)
• Add the gas saved
• Add POC for big statements ("use custom errors lol XD")

Total Gas Saved 453

Adding 24 gas from #33

477