FactoryDAO contest - 0xf15ers's results

Lines of code

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

Vulnerability details

Impact

For the beneficiary to withdraw excess rewards of the pool, pool.totalDepositsWei should be equal to zero. An attacker can always deposit minimum amount to restrict withdraw of rewards to the beneficiary.

Proof of Concept

require(pool.totalDepositsWei == 0, 'Cannot withdraw until all deposits are withdrawn');

Tools Used

Manual Analayis

1. Check for zero tokenBalance before depositTokens()

-In MerkleDropFactory.sol#L59, tokenBalance can be check with require(tokenBalance) > 0 to stop zero depoist.

2. ERC20 transfer return value not checked

• In MerkleVesting.sol#L173, return value of the erc20 transfer should be checked for boolean value to revert on false returned by some tokens.

# Before 
IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);

# After
require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), "fail message");

1. Array length can be cached to save gas in loops

• In PermissionlessBasicPoolFactory.sol#L224 rewards.length is calculed in each iteration. Gas can be saved if the length is cached.

The following is recommended.

uint length = rewards.length
for (uint i = 0; i < length; i++) {
....
}

• same for:

./contracts//PermissionlessBasicPoolFactory.sol:125:        for (uint i = 0; i < rewardTokenAddresses.length; i++) {
./contracts//PermissionlessBasicPoolFactory.sol:151:        for (uint i = 0; i < pool.rewardFunding.length; i++) {
./contracts//PermissionlessBasicPoolFactory.sol:180:        for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
./contracts//PermissionlessBasicPoolFactory.sol:237:        for (uint i = 0; i < rewards.length; i++) {
./contracts//PermissionlessBasicPoolFactory.sol:269:        for (uint i = 0; i < pool.rewardTokens.length; i++) {
./contracts//PermissionlessBasicPoolFactory.sol:286:        for (uint i = 0; i < pool.rewardTokens.length; i++) {
./contracts//MerkleLib.sol:22:        for (uint i = 0; i < proof.length; i += 1) {

2. balances[thisOwner] can also be cached to save twice sload operation.

• In VoterID.sol#L133-L134 2 sload operations can be saved as:

# Before
ownershipMapIndexToToken[thisOwner][balances[thisOwner]] = thisToken;
ownershipMapTokenToIndex[thisOwner][thisToken] = balances[thisOwner];

# After
uint tokenNumber = balances[thisOwner] 
ownershipMapIndexToToken[thisOwner][tokenNumber] = thisToken;
ownershipMapTokenToIndex[thisOwner][thisToken] = tokenNumber;

3. Prefix increment is cheaper than numIdentities = numIdentities + 1

• In VoterID.sol#L130,++numIdentities costs less gas than numIdentities = numIdentities + 1
• Same in https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleEligibility.sol#L47

4. Default value initialization of 0 can be omitted to save gas.

• In https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L16, uint public numTrees = 0 is by default initialized to 0

5. Using inline increment in balances[thisOwner] can save later operations

• In https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L134-L140,

        ownershipMapTokenToIndex[thisOwner][thisToken] = balances[thisOwner];


        // set owner of new token
        owners[thisToken] = thisOwner;
        // increment balances for owner
        balances[thisOwner] = balances[thisOwner] + 1;

can be simmpliifed to

        ownershipMapTokenToIndex[thisOwner][thisToken] = balances[thisOwner]++;
        // set owner of new token
        owners[thisToken] = thisOwner;

6. Reduce the size of error or Use custom errors instead of revert string to save gas in multiple places.

• Custom error introduced from soliidity greater than 0.8.4 can be used.

/contracts/MerkleEligibility.sol:86:        require(msg.sender == gateMaster, "Only gatemaster may call this.");
./contracts/MerkleEligibility.sol:89:        require(isEligible(index, recipient, proof), "Address is not eligible");
./contracts/MerkleIdentity.sol:97:        require(msg.sender == treeAdder, 'Only treeAdder can add trees');
./contracts/MerkleIdentity.sol:124:        require(merkleIndex <= numTrees, 'merkleIndex out of range');
./contracts/MerkleIdentity.sol:127:        require(verifyMetadata(tree.metadataMerkleRoot, tokenId, uri, metadataProof), "The metadata proof could not be verified");
./contracts/MerkleDropFactory.sol:77:        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
./contracts/MerkleDropFactory.sol:90:        require(treeIndex <= numTrees, "Provided merkle index doesn't exist");
./contracts/MerkleDropFactory.sol:92:        require(!withdrawn[destination][treeIndex], "You have already withdrawn your entitled token.");
./contracts/MerkleDropFactory.sol:98:        require(tree.merkleRoot.verifyProof(leaf, proof), "The proof could not be verified.");
./contracts/MerkleDropFactory.sol:107:        require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed");
./contracts/MerkleVesting.sol:89:        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
./contracts/MerkleVesting.sol:106:        require(!initialized[destination][treeIndex], "Already initialized");
./contracts/MerkleVesting.sol:113:        require(tree.rootHash.verifyProof(leaf, proof), "The proof could not be verified.");
./contracts/MerkleVesting.sol:141:        require(initialized[destination][treeIndex], "You must initialize your account first.");
./contracts/MerkleVesting.sol:145:        require(block.timestamp > tranche.lockPeriodEndTime, 'Must wait until after lock period');
./contracts/MerkleVesting.sol:147:        require(tranche.currentCoins >  0, 'No coins left to withdraw');
./contracts/VoterID.sol:123:        require(msg.sender == _minter, 'Only minter may create identity');
./contracts/VoterID.sol:124:        require(owners[thisToken] == address(0), 'Token already exists');
./contracts/VoterID.sol:184:        require(ooner != address(0), 'No such token');
./contracts/VoterID.sol:199:        require(isApproved(msg.sender, tokenId), 'Identity: Unapproved transfer');
./contracts/VoterID.sol:217:        require(checkOnERC721Received(from, to, tokenId, data), "Identity: transfer to non ERC721Receiver implementer");
./contracts/VoterID.sol:239:        require(isApproved(msg.sender, tokenId), 'Identity: Not authorized to approve');
./contracts/VoterID.sol:240:        require(holder != approved, 'Identity: Approving self not allowed');
./contracts/VoterID.sol:262:        require(holder != address(0), 'Identity: Invalid tokenId');
./contracts/VoterID.sol:305:        require(owners[tokenId] == from, "Identity: Transfer of token that is not own");
./contracts/VoterID.sol:306:        require(to != address(0), "Identity: transfer to the zero address");
./contracts/VoterID.sol:437:        require(_index < numIdentities, 'Invalid token index');
./contracts/VoterID.sol:449:        require(_index < balances[_address], 'Index out of range');
./contracts/VoterID.sol:450:        require(_address != address(0), 'Cannot query zero address');
./contracts/SpeedBumpPriceGate.sol:67:        require(msg.value >= price, 'Please send more ETH');
./contracts/SpeedBumpPriceGate.sol:80:            require(sent, 'ETH transfer failed');
./contracts/MerkleResistor.sol:82:        require(pctUpFront < 100, 'pctUpFront >= 100');
./contracts/MerkleResistor.sol:83:        require(minEndTime < maxEndTime, 'minEndTime must be less than maxEndTime');
./contracts/MerkleResistor.sol:121:        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
./contracts/MerkleResistor.sol:136:        require(msg.sender == destination, 'Can only initialize your own tranche');
./contracts/MerkleResistor.sol:138:        require(!initialized[destination][treeIndex], "Already initialized");
./contracts/MerkleResistor.sol:144:        require(tree.merkleRoot.verifyProof(leaf, proof), "The proof could not be verified.");
./contracts/MerkleResistor.sol:149:        require(valid, 'Invalid vesting schedule');
./contracts/MerkleResistor.sol:171:        require(initialized[destination][treeIndex], "You must initialize your account first.");
./contracts/MerkleResistor.sol:175:        require(tranche.currentCoins >  0, 'No coins left to withdraw');
./contracts/MerkleResistor.sol:204:        require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), 'Token transfer failed');
./contracts/FixedPricePassThruGate.sol:48:        require(msg.value >= gate.ethCost, 'Please send more ETH');
./contracts/FixedPricePassThruGate.sol:54:            require(sent, 'ETH transfer failed');
./contracts/PermissionlessBasicPoolFactory.sol:112:        require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');
./contracts/PermissionlessBasicPoolFactory.sol:148:        require(success, 'Token deposits failed');
./contracts/PermissionlessBasicPoolFactory.sol:159:        require(pool.id == poolId, 'Uninitialized pool');
./contracts/PermissionlessBasicPoolFactory.sol:160:        require(receipt.id == receiptId, 'Uninitialized receipt');
./contracts/PermissionlessBasicPoolFactory.sol:182:        require(pool.id == poolId, 'Uninitialized pool');
./contracts/PermissionlessBasicPoolFactory.sol:183:        require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');
./contracts/PermissionlessBasicPoolFactory.sol:184:        require(block.timestamp < pool.endTime, 'Cannot deposit after pool ends');
./contracts/PermissionlessBasicPoolFactory.sol:185:        require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');
./contracts/PermissionlessBasicPoolFactory.sol:199:        require(success, 'Token transfer failed');
./contracts/PermissionlessBasicPoolFactory.sol:211:        require(pool.id == poolId, 'Uninitialized pool');
./contracts/PermissionlessBasicPoolFactory.sol:213:        require(receipt.id == receiptId, 'Can only withdraw real receipts');
./contracts/PermissionlessBasicPoolFactory.sol:214:        require(receipt.owner == msg.sender || block.timestamp > pool.endTime, 'Can only withdraw your own deposit');
./contracts/PermissionlessBasicPoolFactory.sol:215:        require(receipt.timeWithdrawn == 0, 'Can only withdraw once per receipt');
./contracts/PermissionlessBasicPoolFactory.sol:234:        require(success, 'Token transfer failed');
./contracts/PermissionlessBasicPoolFactory.sol:244:        require(pool.id == poolId, 'Uninitialized pool');
./contracts/PermissionlessBasicPoolFactory.sol:245:        require(pool.totalDepositsWei == 0, 'Cannot withdraw until all deposits are withdrawn');
./contracts/PermissionlessBasicPoolFactory.sol:246:        require(block.timestamp > pool.endTime, 'Contract must reach maturity');
./contracts/PermissionlessBasicPoolFactory.sol:254:        require(success, 'Token transfer failed');
./contracts/PermissionlessBasicPoolFactory.sol:263:        require(pool.id == poolId, 'Uninitialized pool');
./contracts/PermissionlessBasicPoolFactory.sol:271:        require(success, 'Token transfer failed');
./contracts/PermissionlessBasicPoolFactory.sol:315:        require(msg.sender == globalBeneficiary, 'Only globalBeneficiary can set tax');
./contracts/PermissionlessBasicPoolFactory.sol:316:        require(newTaxPerCapita < 1000, 'Tax too high');  

7. Setting visibility to external is cheaper than public

• Functions that are not called inside the contract can be set to external to save gas

#MerkleDropFactory.sol function addMerkleTree(bytes32 newRoot, bytes32 ipfsHash, address tokenAddress, uint tokenBalance) public {
#MerkleDropFactory.sol function withdraw(uint treeIndex, address destination, uint value, bytes32[] memory proof) public {
#MerkleVesting.sol  addMerkleRoot(bytes32 newRoot, bytes32 ipfsHash, address tokenAddress, uint tokenBalance) public {
#MerkleVesting.sol function withdraw(uint treeIndex, address destination) public {
#MerkleResistor.sol function addMerkleTree(bytes32 newRoot, bytes32 ipfsHash, uint minEndTime, uint maxEndTime, uint pctUpFront, address tokenAddress, uint tokenBalance) public {
..........