FactoryDAO contest - 0xkatana's results

[L-01] Use safeTransfer/safeTransferFrom consistently

A require() statement that checks the return value of token transfers should be used to prevent silent transfer failures with non-compliant ERC20 tokens. Failure to do so can cause silent failures of transfers and affect token accounting in contract, possibly causing loss of value for the user or the protocol.

Proof of Concept

One unsafe transfer() exists without return value checks: https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L173

Recommended Mitigation Steps

Use require statements to check return values consistently or replace transfer/transferFrom with safeTransfer/safeTransferFrom from Open Zeppelin's SafeERC20 library

[L-02] isContract logic can be bypassed

The logic in isContract can be bypassed. It uses the same approached described in https://solidity-by-example.org/hacks/contract-size

Proof of Concept

The logic that can be bypassed is for isContract https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L343

Recommended Mitigation Steps

One solution that works for now is to check tx.origin == msg.sender. This is true for EOAs and false for contracts, but this can change with later EIPs.

[G-01] Short require strings save gas

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L141 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L145 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L214 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L217 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L305 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L306

Recommended Mitigation Steps

Shorten all require strings to less than 32 characters

[G-02] Use != 0 instead of > 0

Using > 0 uses slightly more gas than using != 0. Use != 0 when comparing uint variables to zero, which cannot hold values below zero

Locations where this was found include https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L348 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/SpeedBumpPriceGate.sol#L77 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/FixedPricePassThruGate.sol#L51

Recommended Mitigation Steps

Replace > 0 with != 0 to save gas

[G-03] Use Solidity errors instead of require

Solidity errors introduced in version 0.8.4 can save gas on revert conditions https://blog.soliditylang.org/2021/04/21/custom-errors/ https://twitter.com/PatrickAlphaC/status/1505197417884528640

Error messages are not used at all in the contracts. Some example of require usage that can be replaced with errors includes https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L141 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L145 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L214

Recommended Mitigation Steps

Replace require blocks with new solidity errors described in https://blog.soliditylang.org/2021/04/21/custom-errors/

[G-04] Use prefix not postfix in loops

Using a prefix increment (++i) instead of a postfix increment (i++) saves gas for each loop cycle and so can have a big gas impact when the loop executes on a large number of elements.

There are many examples of this in for loops https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L115 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L141 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L168 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L224 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L249 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L266

Recommended Mitigation Steps

Use prefix not postfix to increment in a loop

[G-05] Use simple comparison in if statement

The comparison operators >= and <= use more gas than >, <, or ==. Replacing the >= and ≤ operators with a comparison operator that has an opcode in the EVM saves gas

The existing code is https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L179-L185

if (block.timestamp >= tranche.endTime) {
    currentWithdrawal = tranche.currentCoins;
} else {
    // compute allowed withdrawal
    // secondsElapsedSinceLastWithdrawal * coinsPerSecond == coinsAccumulatedSinceLastWithdrawal
    currentWithdrawal = (block.timestamp - tranche.lastWithdrawalTime) * tranche.coinsPerSecond;
}

A simple comparison can be used for gas savings by reversing the logic

if (block.timestamp < tranche.endTime) {
    // compute allowed withdrawal
    // secondsElapsedSinceLastWithdrawal * coinsPerSecond == coinsAccumulatedSinceLastWithdrawal
    currentWithdrawal = (block.timestamp - tranche.lastWithdrawalTime) * tranche.coinsPerSecond;
} else {
    currentWithdrawal = tranche.currentCoins;
}

The same change can be used here https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L153-L158

Recommended Mitigation Steps

Replace the comparison operator and reverse the logic to save gas using the suggestions above

[G-06] Redundant zero initialization

Solidity does not recognize null as a value, so uint variables are initialized to zero. Setting a uint variable to zero is redundant and can waste gas.

There was one place where an int is initialized to zero https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleDropFactory.sol#L17 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/VoterID.sol#L69 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleEligibility.sol#L31 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L24 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L176 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L16 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L150 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L115 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L141 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L168 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L224 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L249 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L266

Recommended Mitigation Steps

Remove the redundant zero initialization uint256 i; instead of uint256 i = 0;

[G-07] Cache array length before loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop. This saves gas.

This was found in many places https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleLib.sol#L22 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L115 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L141 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L168 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L224 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L249 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L266

Recommended Mitigation Steps

Cache the array length before the for loop