search

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

1 stars 1 forks source link

Gas Optimizations #215

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

1. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

PermissionlessBasicPoolFactory

MerkleLib.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to cache the array length outside of for loop.

2. Long revert error messages

Impact

Shortening revert error messages to fit in 32 bytes will decrease gas costs for deployment and gas costs when the revert condition has been met.

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

MerkleIdentity.sol:

VoterID.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to decrease revert messages to maximum 32 bytes. Alternatively custom error types should be used.

3. Use custom errors instead of revert strings to save gas

Impact

Usage of Custom Errors reduces the gas cost.

Proof of Concept

PermissionlessBasicPoolFactory.sol:

    require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');
    require(success, 'Token deposits failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(receipt.id == receiptId, 'Uninitialized receipt');
    require(pool.id == poolId, 'Uninitialized pool');
    require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');
    require(block.timestamp < pool.endTime, 'Cannot deposit after pool ends');
    require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');
    require(success, 'Token transfer failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(receipt.id == receiptId, 'Can only withdraw real receipts');
    require(receipt.owner == msg.sender || block.timestamp > pool.endTime, 'Can only withdraw your own deposit');
    require(receipt.timeWithdrawn == 0, 'Can only withdraw once per receipt');
    require(success, 'Token transfer failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(pool.totalDepositsWei == 0, 'Cannot withdraw until all deposits are withdrawn');
    require(block.timestamp > pool.endTime, 'Contract must reach maturity');
    require(success, 'Token transfer failed');
    require(pool.id == poolId, 'Uninitialized pool');
    require(success, 'Token transfer failed');
    require(msg.sender == globalBeneficiary, 'Only globalBeneficiary can set tax');
    require(newTaxPerCapita < 1000, 'Tax too high');

MerkleDropFactory.sol:

    require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
    require(treeIndex <= numTrees, "Provided merkle index doesn't exist");
    require(!withdrawn[destination][treeIndex], "You have already withdrawn your entitled token.");
    require(tree.merkleRoot.verifyProof(leaf, proof), "The proof could not be verified.");
    require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed");

MerkleVesting.sol:

    require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
    require(!initialized[destination][treeIndex], "Already initialized");
    require(tree.rootHash.verifyProof(leaf, proof), "The proof could not be verified.");
    require(initialized[destination][treeIndex], "You must initialize your account first.");
    require(block.timestamp > tranche.lockPeriodEndTime, 'Must wait until after lock period');
    require(tranche.currentCoins >  0, 'No coins left to withdraw');

MerkleResistor.sol:

    require(pctUpFront < 100, 'pctUpFront >= 100');
    require(minEndTime < maxEndTime, 'minEndTime must be less than maxEndTime');
    require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
    require(msg.sender == destination, 'Can only initialize your own tranche');
    require(!initialized[destination][treeIndex], "Already initialized");
    require(tree.merkleRoot.verifyProof(leaf, proof), "The proof could not be verified.");
    require(valid, 'Invalid vesting schedule');
    require(initialized[destination][treeIndex], "You must initialize your account first.");
    require(tranche.currentCoins >  0, 'No coins left to withdraw');
    require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), 'Token transfer failed');

FixedPricePassThruGate.sol:

    require(msg.value >= gate.ethCost, 'Please send more ETH');
    require(sent, 'ETH transfer failed')

SpeedBumpPriceGate.sol:

    require(msg.value >= price, 'Please send more ETH');
    require(sent, 'ETH transfer failed');

MerkleEligibility.sol:

    require(msg.sender == gateMaster, "Only gatemaster may call this.");
    require(isEligible(index, recipient, proof), "Address is not eligible");

MerkleIdentity.sol:

    require (msg.sender == management, 'Only management may call this');
    require(msg.sender == treeAdder, 'Only treeAdder can add trees');
    require(merkleIndex <= numTrees, 'merkleIndex out of range');
    require(verifyMetadata(tree.metadataMerkleRoot, tokenId, uri, metadataProof), "The metadata proof could not be verified");

VoterID.sol:

    require (msg.sender == _owner_, 'Identity: Only owner may call this');
    require(msg.sender == _minter, 'Only minter may create identity');
    require(owners[thisToken] == address(0), 'Token already exists');
    require(ooner != address(0), 'No such token');
    require(isApproved(msg.sender, tokenId), 'Identity: Unapproved transfer');
    require(checkOnERC721Received(from, to, tokenId, data), "Identity: transfer to non ERC721Receiver implementer");
    require(isApproved(msg.sender, tokenId), 'Identity: Not authorized to approve');
    require(holder != approved, 'Identity: Approving self not allowed');
    require(holder != address(0), 'Identity: Invalid tokenId');
    require(owners[tokenId] == from, "Identity: Transfer of token that is not own");
    require(to != address(0), "Identity: transfer to the zero address");
    require(_index < numIdentities, 'Invalid token index');
    require(_index < balances[_address], 'Index out of range');
    require(_address != address(0), 'Cannot query zero address');

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add Custom Errors.

4. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleLib.sol:

MerkleEligibility.sol:

VoterID.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

5. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleLib.sol:

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

FixedPricePassThruGate.sol:

SpeedBumpPriceGate.sol:

MerkleEligibility.sol:

MerkleIdentity.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended wrap incrementing of i with unchecked block:

for (uint256 i; i < numWarriors; ) {
  _mint(msg.sender);
  unchecked { ++i };
}

6. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

PermissionlessBasicPoolFactory.sol

MerkleLib.sol:

MerkleDropFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

MerkleEligibility.sol:

VoterID.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to remove explicit initializations with default values.

7. Pack integer values

Impact

Proof of Concept

PermissionlessBasicPoolFactory.sol:

MerkleResistor.sol:

MerkleVesting.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to pack listed values in order to consume less storage and thus gas.

illuzen commented 2 years ago

all duplicates