FactoryDAO contest - PPrieditis's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/SpeedBumpPriceGate.sol#L65-L82

Vulnerability details

Impact

NFTs should be sold for their best price without descending into a gas race: https://github.com/code-423n4/2022-05-factorydao#mint Due to SpeedBumpPriceGate.sol function passThruGate() code users will pay more than required for NFT Minting. User will pay msg.value and not the NFT "price". It is very likely that user will send more ether than current price because price increase can happen between purchase transaction transmission and actual processing. Users should be refunded if they send more ether than required in order to decrease gas race possibility.

Recommended Mitigation Steps

1. Pay NFT price and not msg.value replace: (bool sent, bytes memory data) = gate.beneficiary.call{value: msg.value}(""); https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/SpeedBumpPriceGate.sol#L79 with: (bool sent, bytes memory data) = gate.beneficiary.call{value: price}("");
2. Refund unspent msg.value back to user

Judge has assessed an item in Issue #198 as Medium risk. The relevant finding follows:

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:

• OZ uses internal, private functions however custom library has only public functions
• OZ uses assembly with mstore()

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:

• setGlobalTax() https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L314

MerkleIdentity.sol:

• setManagement() https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L60
• setTreeAdder() https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L67
• setIpfsHash() https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L75

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

Title: Use immutable variables

Impact

For any variable that is only set once, either use a constant or immutable to save gas. The compiler does not reserve a storage slot for these variables, and every occurrence is replaced by the respective value (Source: https://docs.soliditylang.org/en/v0.6.5/contracts.html#constant-and-immutable-state-variables)

Recommended Mitigation Steps

Change variables as immutable.

globalBeneficiary PermissionlessBasicPoolFactory.sol: https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L51

gateMaster MerkleEligibility.sol: https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleEligibility.sol#L16

Title: Use Solidity v0.8.4 because of Solidity Custom Errors

Impact

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Custom errors decrease both deploy and runtime gas costs. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/

OpenZeppelin is also planing to use custom errors starting with next major release 5.0. Source release v5.0: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2961 Source OZ custom error issue: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2839

Recommended Mitigation Steps

Refactor code and use Solidity custom errors.

Title: Unused .call() return data

Impact

Removing unused return variable improves code readability and can improve gas costs

Recommended Mitigation Steps

Change: (bool sent, bytes memory data) = gate.beneficiary.call{value: msg.value}(""); To: (bool sent, ) = gate.beneficiary.call{value: msg.value}(""); https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/SpeedBumpPriceGate.sol#L79