search

code-423n4 / 2022-05-factorydao-findings

1 stars 1 forks source link

QA Report #198

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Title: Do not use a custom library for merkle proofs

Impact

It is better to use widely used public libraries than custom. Source of MerkleLib.sol library is not known however functionality matches OpenZeppelin MerkleProof.sol library. OZ library should uses less gas because:

Recommended Mitigation Steps

Replace: https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleLib.sol with: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/MerkleProof.sol

Title: PermissionlessBasicPoolFactory.sol deposit() is not possible before pool.startTime

Impact

Pool creator allocates enough rewards to cover a case if pool has maximumDepositWei from pool.startTime to pool.endTime so it should be made possible to have totalDepositsWei high as possible at pool.startTime. Pool creator wants to attract as many deposits as possible and pool will attract more deposits if it will be possible to deposit before startTime.

Recommended Mitigation Steps

Change the logic of deposit() https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L180

It should be allowed to make a deposit before pool.startTime and if user makes a deposit before startTime then pool.startTime should be used for receipt.timeDeposited.

Title: PermissionlessBasicPoolFactory.sol tokens with fee on transfer are not supported

Impact

There are ERC20 tokens that charge fee for every transfer() or transferFrom(). In the current implementation, PermissionlessBasicPoolFactory.sol assumes that the received amount is the same as the transfer amount, and uses it to calculate rewards. As a result, in withdraw(), later users may not be able to successfully withdraw their tokens, as it may revert at L230 for insufficient balance. https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L230

Recommended Mitigation Steps

Consider comparing before and after balance to get the actual transferred amount.

Title: Don't use magic numbers

Impact

Magic Numbers obscure the purpose of the function and unnecessarily lead to potential error if the constants are changed during development.

Recommended Mitigation Steps

Create new constants.

1000 constant

/// @param _globalTaxPerCapita the amount of the rewards that goes to the globalBeneficiary * 1000 (perCapita) https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L74

uint tax = (pool.taxPerCapita * rewards[i]) / 1000; https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L227

require(newTaxPerCapita < 1000, 'Tax too high'); https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L316

1e18 constant

rewardsLocal[i] = (secondsDiff pool.rewardsWeiPerSecondPerToken[i] receipt.amountDepositedWei) / 1e18; https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L169

return pool.rewardsWeiPerSecondPerToken[rewardIndex] pool.maximumDepositWei (pool.endTime - pool.startTime) / 1e18; https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L282

Title: Not emitting event for important state change

Impact

The system doesn't record historical state changes.

Recommended Mitigation Steps

Add evets. PermissionlessBasicPoolFactory.sol:

MerkleIdentity.sol:

Title: Not checking transfer() return value at MerkleVesting.sol withdraw()

Impact

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Furthermore, some tokens (like USDT) don't correctly implement the ERC20 standard and don't return a boolean.
It is necessary to use a consistent approach across all the contract, for example, transfer() return value is checked at PermissionlessBasicPoolFactory.sol, MerkleDropFactory.sol

Recommended Mitigation Steps

Check return value: IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal); https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L173 the same way as at: require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed"); https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleDropFactory.sol#L107

Title: SpeedBumpPriceGate.sol passThruGate() msg.value validation duplicate

Impact

SpeedBumpPriceGate.sol function passThruGate() checks if "msg.value > getCost(index)" so second validation "msg.value > 0" is unnecessary.

Recommended Mitigation Steps

Remove redundant validation of "if (msg.value > 0) {" https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/SpeedBumpPriceGate.sol#L65

illuzen commented 2 years ago

All duplicates except MerkleLib...