FactoryDAO contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

Contracts should clarify what is the intended behavior for Merkle trees with multiple leafs with the same address.

Recommended Mitigation Steps

There is 2 possible behaviors:

• either - what is currently done - you only authorize one claim per address, in which case the multiple leaf are here to give users a choice - for example you could use MerkleVesting to give users the choice between 2 sets of vesting parameters and have something close to MerkleResistor.
• either you use a mapping based on the leaf to store if a leaf has been claimed or not.

This behavior should be clarified in the comments at least, and made clear to merkle tree builders.

[L-01]

For maximum compatibility, use safeTransferFrom as you don’t know if tokens returns boolean or use revert statements. Example:

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

Reference:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/3bdf4bfd29b145288ec741bef259a384007ed5cf/contracts/token/ERC20/utils/SafeERC20.sol#L21

[GAS - 01]


spentTokens variable is not used for accounting so could be removed and tracked on chain to save gas.

Code: https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L25