FactoryDAO contest - fatherOfBlocks's results

Lines of code

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

Vulnerability details

Impact

When the transfer is made in the withdraw() function, it is not validated if the transfer was done correctly. This could be a conflict since not being able to perform it would return a false and that case would not be handled, the most common is to revert.

Recommended Mitigation Steps

The recommendation is to wrap the transfer with a require, as is done in MerkleDropFactory.sol for example.

PermissionlessBasicPoolFactory.sol

• L121: If maxDeposit is zero, it will never be possible to execute deposit(), therefore a DoS would be performed and nothing would work in that pool.
• L180: It should be checked that amount != 0, otherwise all the code would be executed and no deposit would be generated.
• L215/218: If receipt.timeWithdrawn can only have two values, it would make much more sense to just use a bool.

MerkleVesting.sol

• L139/L104: I think that in the withdraw() function the signature (address destination, uint treeIndex) makes more sense since that is how the data is used. The same goes for the initialize() function, although there are more input parameters in the signature.

MerkleResistor.sol

• L134/L136: In the initialize() function, if the destination can only be the msg.sender, it would be easier not to request that input parameter and use the msg.sender without the require directly.

MerkleEligibility.sol

• L36/L86: The variable in storage: gateMaster, is only set in the constructor, if it is set incorrectly the function passThruGate() has a DoS therefore it must be deployed again. Minimally I think it should be validated that in the constructor _gateMaster != 0, but maybe it could be evaluated that there is a setter.

VoterID.sol

• L108/L151: The variable in storage: owner, is only set in the constructor and in the setOwner() function, but it is never evaluated if owner != 0, if set to zero, they would have a DoS in setOwner() and setTokenURI().
• L108/L123: The variable in storage: _minter, is only set in the constructor, but is never evaluated if _minter!= 0, if set to zero, they would have a DoS in createIdentityFor().
• L449/L450: In the tokenOfOwnerByIndex() function, the correct thing would be to first validate _address != address(0) and then _index < balances[_address].

PermissionlessBasicPoolFactory.sol

• L115/141/168/224/266: Gas would be saved if instead of i++, it was ++i without marking.
• L115/141/168/224/266: Gas would be saved if i = 0 is not set.
• L166: It is not possible for nowish to be smaller than receipt.timeDeposited, since if or if deposit has to be executed first so that it does not reverse the require of lines 159 and 160.
• L168: It would be better to use rewardsLocal.length, instead of pool.rewardsWeiPerSecondPerToken.length, since the variable in memory generates less gas cost than in storage (600 gas less, in remix tests).

MerkleLib.sol

• L22: You could save 940 gas if instead of i += 1 in the for, you put unchecked { ++i;}
• L22: Gas would be saved if i = 0 is not set.

MerkleDropFactory.sol

• L17:Gas is saved if numTrees is not set to any value.

MerkleVesting.sol

• L16: Gas is saved if numTrees is not set to any value.
• L104: It should be that in the initialize() function startTime is smaller than endTime. It should also be validated that endTime != startTime, otherwise it would revert to line 117 uint coinsPerSecond = totalCoins / (endTime - startTime);
• L150: Gas is saved if currentWithdrawal is not set to any value.

MerkleResistor.sol

• L24: Gas is saved if numTrees is not set to any value.
• L176: Gas is saved if currentWithdrawal is not set to any value.

MerkleIdentity.sol

• L124: In the withdraw() function, a validation of an input parameter is performed in the middle of the function, it would be more optimal to do it at the beginning, since if it reverts, less gas is spent than it is now.

• L127: In the withdraw() function, a validation is performed with verifyMetadata(), it would be more optimal to do it just after the variable tree = merkleTrees[merkleIndex] is read from storage; since if it reverses, less gas is spent than it is now.

• L142/L143: You could save a little gas, if instead of returning a variable directly, you return the function call, in the getPrice() function.

• L118/L121: In the withdraw() function, the local variable id is only used once, therefore, it would be less expensive and more explicit (since the name "id" is not at all descriptive) it would be to do it like this: IVoterID(tree.nftAddress ).createIdentityFor(msg.sender, tokenId, uri);

MerkleEligibility.sol

• L31: Gas is saved if numTrees is not set to any value.

VoterID.sol

• L69: Gas is saved if numTrees is not set to any value.
• L97: The ownerOnly() modifier could be a view function and that way you could save a lot of gas (20,000 gas tested in remix).