FactoryDAO contest - ilan's results

NC01 internal function name should start with _

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L137-L137

the convention should be followed of starting internal function names with a underscore.

NC02 too many whitespaces between operator and number

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L147-L147

change 2 whitespaces to 1.

require(tranche.currentCoins > 0, 'No coins left to withdraw') -> require(tranche.currentCoins > 0, 'No coins left to withdraw')

NC03 non consistant Quotation mark usage

should use double quotation consistantly

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L145-L145

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L147

NC04 some functions declarations exceed recommended maximum line length

From the solidity style guide:

Keeping lines under the PEP 8 recommendation to a maximum of 79 (or 99) characters helps > readers easily parse the code.

Example instance

Recommendation make the function declarations over multiple lines

function name(
        type param1,
        type param2,
        type param3
    ) visibilty {
        body
    }

L01 Require endTime > startTime or else 0 division could occur

initialize can be called with endTime == startTime and that will cause a zero division error. I recommend to require that endTime > startTime or take care of this case seperately

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L117-L117

L02 Should wrap .transfer with require

like transferTo(), also .transfer() returns bool which can be true or false. So it is recommended to check that true is returned before by wrapping transfer with a require statement.

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/MerkleVesting.sol#L173-L173

L03 priceIncreaseFactor, priceIncreaseDenominator should have validity checks

The price increase values should be checked:

1. priceIncreaseFactor > priceIncreaseDenominator, otherwise the price could decrease / constant
2. priceIncreaseDenominator > 0, otherwise obviously the denominator is invalid and we'll get division by 0.

add the cheks to SpeedBumpPriceGate.addGate

L04 Broken comment

VoterID#L10 contract comment is incomplete.

L05 Initial tax can be over 100%

when setting _globalTaxPerCapita in the constructor of PermissionlessBasicPoolFactory there is no check that _globalTaxPerCapita < 1000 as there is in setGlobalTax. Hence, the initial tax can be more than 100 percent which will break withdraw().

I recommend adding require(newTaxPerCapita < 1000, 'Tax too high'); to the constructor.

G01 Change i++ to ++i

i++ is generally more expensive because it must increment a value and return the old value, so it may require holding two numbers in memory. ++i only uses one number in memory.

Example:

for (uint i = 0; i < rewardTokenAddresses.length; i++) -> for (uint i = 0; i < rewardTokenAddresses.length; ++i)

PermissionlessBasicPoolFactory.addPool#L115

G02 Loop storage/memory acess cache

Calling storage/memory variable which does not change, every loop iteration, is a wrong pattern and wastes gas.

Example:

for (uint i = 0; i < rewardTokenAddresses.length; i++)

->

length = rewardTokenAddresses.length
for (uint i = 0; i < length; ++i)

PermissionlessBasicPoolFactory.addPool#L115

G03 No need to explicitly initialize variables with default values

If a variable is not initialized it is automatically set to the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Example:

uint i = 0 -> uint i

PermissionlessBasicPoolFactory.addPool#L115

G04 functions should be decalred external

public functions that are never called by the contract should be declared external to save gas.

for example MerkleDropFactory.withdraw

G05 Use calldata

Use calldata instead of memory for external functions where the function argument is read-only. Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.