FactoryDAO contest - oyc_109's results

No Transfer Ownership Pattern

description

The current ownership transfer process involves the current owner calling setOwner(). If the nominated EOA account is not a valid account, or is the zero address, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the ownerOnly() modifier.

Recommend considering implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

findings

File: VoterID.sol
151:     function setOwner(address newOwner) external ownerOnly {
152:         address oldOwner = _owner_;
153:         _owner_ = newOwner;
154:         emit OwnerUpdated(oldOwner, newOwner);
155:     }

use calldata instead of memory

description

Use calldata instead of memory for function parameters saves gas In some cases, having function arguments in calldata instead of memory is more optimal.

Consider the following generic example:

    contract C {
        function add(uint[] memory arr) external returns (uint sum) {
            uint length = arr.length;
            for (uint i = 0; i < arr.length; i++) {
                sum += arr[i];
            }
        }
    }

In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:

    contract C {
        function add(uint[] calldata arr) external returns (uint sum) {
            uint length = arr.length;
            for (uint i = 0; i < arr.length; i++) {
                sum += arr[i];
            }
        }
    }

In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.

Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.

In short, use calldata instead of memory if the function argument is only read.

Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.

findings

File: PermissionlessBasicPoolFactory.sol
92:     function addPool (
93:         uint startTime,
94:         uint maxDeposit,
95:         uint[] memory rewardsWeiPerSecondPerToken,
96:         uint programLengthDays,
97:         address depositTokenAddress,
98:         address excessBeneficiary,
99:         address[] memory rewardTokenAddresses,
100:         bytes32 ipfsHash,
101:         bytes32 name
102:     )

File: MerkleLib.sol
17: function verifyProof(bytes32 root, bytes32 leaf, bytes32[] memory proof) public pure returns (bool) {

File: MerkleVesting.sol
104: function initialize(uint treeIndex, address destination, uint totalCoins, uint startTime, uint endTime, uint lockPeriodEndTime, bytes32[] memory proof) external {

File: MerkleIdentity.sol
116: function withdraw(uint merkleIndex, uint tokenId, string memory uri, bytes32[] memory addressProof, bytes32[] memory metadataProof) external payable {
152: function isEligible(uint merkleIndex, address recipient, bytes32[] memory proof) public view returns (bool) {
163: function verifyMetadata(bytes32 root, uint tokenId, string memory uri, bytes32[] memory proof) public pure returns (bool) {

File: MerkleEligibility.sol
70: function isEligible(uint index, address recipient, bytes32[] memory proof) public override view returns (bool eligible) {

File: MerkleEligibility.sol
85: function passThruGate(uint index, address recipient, bytes32[] memory proof) external override {

Long Revert Strings

description

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

If the contract(s) in scope allow using Solidity >=0.8.4, consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

findings

File: PermissionlessBasicPoolFactory.sol
112: require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');
315: require(msg.sender == globalBeneficiary, 'Only globalBeneficiary can set tax');

File: MerkleDropFactory.sol
90: require(treeIndex <= numTrees, "Provided merkle index doesn't exist");
92: require(!withdrawn[destination][treeIndex], "You have already withdrawn your entitled token.");

File: MerkleVesting.sol
141: require(initialized[destination][treeIndex], "You must initialize your account first.");
145: require(block.timestamp > tranche.lockPeriodEndTime, 'Must wait until after lock period');

File: MerkleResistor.sol
83: require(minEndTime < maxEndTime, 'minEndTime must be less than maxEndTime');
171: require(initialized[destination][treeIndex], "You must initialize your account first.");

File: MerkleIdentity.sol
127: require(verifyMetadata(tree.metadataMerkleRoot, tokenId, uri, metadataProof), "The metadata proof could not be verified");

File: VoterID.sol
217: require(checkOnERC721Received(from, to, tokenId, data), "Identity: transfer to non ERC721Receiver implementer");

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

File: PermissionlessBasicPoolFactory.sol
115: for (uint i = 0; i < rewardTokenAddresses.length; i++) {
168: for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
224: for (uint i = 0; i < rewards.length; i++) {
249: for (uint i = 0; i < pool.rewardTokens.length; i++) {
266: for (uint i = 0; i < pool.rewardTokens.length; i++) {

File: MerkleLib.sol
22: for (uint i = 0; i < proof.length; i += 1) {

File: MerkleDropFactory.sol
17: uint public numTrees = 0;

File: MerkleVesting.sol
16: uint public numTrees = 0;
150: uint currentWithdrawal = 0;

File: MerkleResistor.sol
24: uint public numTrees = 0;
176: uint currentWithdrawal = 0;

File: VoterID.sol
69: uint public numIdentities = 0;

using prefix increments save gas

description

Prefix increments are cheaper than postfix increments, eg ++i rather than i++

findings

File: PermissionlessBasicPoolFactory.sol
115: for (uint i = 0; i < rewardTokenAddresses.length; i++) {
168: for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
224: for (uint i = 0; i < rewards.length; i++) {
249: for (uint i = 0; i < pool.rewardTokens.length; i++) {
266: for (uint i = 0; i < pool.rewardTokens.length; i++) {

do not cache variable used only once

description

for variables only used once, changing it to inline saves gas

findings

File: PermissionlessBasicPoolFactory.sol
166: uint secondsDiff = nowish - receipt.timeDeposited;
198: bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);

File: MerkleVesting.sol
109: bytes32 leaf = keccak256(abi.encode(destination, totalCoins, startTime, endTime, lockPeriodEndTime));

File: MerkleResistor.sol
140: bytes32 leaf = keccak256(abi.encode(destination, minTotalPayments, maxTotalPayments));
246: uint paymentSlope = (maxTotalPayments - minTotalPayments) * PRECISION / (tree.maxEndTime - tree.minEndTime);
264: uint startTime = block.timestamp + vestingTime - (totalCoins / coinsPerSecond);

File: MerkleEligibility.sol
73:         bytes32 leaf = keccak256(abi.encode(recipient));
75:         bool countValid = timesWithdrawn[index][recipient] < gate.maxWithdrawalsAddress;

cache in variables instead of loading

description

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas).

findings

File: PermissionlessBasicPoolFactory.sol
224: for (uint i = 0; i < rewards.length; i++) { //@audit rewards.length should be cached
249: for (uint i = 0; i < pool.rewardTokens.length; i++) { //@audit pool.rewardTokens.length should be cached
266: for (uint i = 0; i < pool.rewardTokens.length; i++) { //@audit pool.rewardTokens.length should be cached

File: MerkleLib.sol
22: for (uint i = 0; i < proof.length; i += 1) { //@audit proof.length should be cached

named returns and a return statement isn’t necessary

description

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

findings

the function isEligible uses both a named return and return statement

File: MerkleEligibility.sol
70:     function isEligible(uint index, address recipient, bytes32[] memory proof) public override view returns (bool eligible) {
71:         Gate memory gate = gates[index];
72:         // We need to pack the 20 bytes address to the 32 bytes value, so we call abi.encode
73:         bytes32 leaf = keccak256(abi.encode(recipient));
74:         // Check the per-address count first
75:         bool countValid = timesWithdrawn[index][recipient] < gate.maxWithdrawalsAddress;
76:         // Then check global count and merkle proof
77:         return countValid && gate.totalWithdrawals < gate.maxWithdrawalsTotal && gate.root.verifyProof(leaf, proof);
78:     }

the function getCost uses both a named return and return statement

File: FixedPricePassThruGate.sol
38:     function getCost(uint index) override external view returns (uint _ethCost) {
39:         Gate memory gate = gates[index];
40:         return gate.ethCost;
41:     }

the function getCost uses both a named return and return statement

File: SpeedBumpPriceGate.sol
50:     function getCost(uint index) override public view returns (uint _ethCost) {
51:         Gate memory gate = gates[index];
52:         // compute the linear decay
53:         uint decay = gate.decayFactor * (block.number - gate.lastPurchaseBlock);
54:         // gate.lastPrice - decay < gate.priceFloor (left side could underflow)
55:         if (gate.lastPrice < decay + gate.priceFloor) {
56:             return gate.priceFloor;
57:         } else {
58:             return gate.lastPrice - decay;
59:         }
60:     }