FactoryDAO contest - joestakey's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L173

Vulnerability details

ERC20: TRANSFER() RETURN VALUE NOT CHECKED

the ERC20 .transfer() method is used in MerkleVesting.sol, but its return value is not checked. It is good to add a require() statement that checks the return value of token transfers, or to use something like OpenZeppelin’s safeTransfer. Failure to do so will cause silent failures of transfers, which will be confusing for users calling these functions, and may result in loss of funds.

Impact

Medium

Proof Of Concept

Instances include:

MerkleVesting:173: IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);

If the transfer fails, the function will not revert. As the tree balance is updated line 167, this would result in a user not being able to retrieve their tokens.

Tools Used

Manual Analysis

Recommended Mitigation Steps

use safeTransfer or require() consistently.

QA Report

Table of Contents

• summary
• Related data should be grouped in struct
• Typos
• Uint256 alias
• Unchecked inputs
• Setters should emit an event

summary

Few vulnerabilities were found examining the contracts. The main concerns are with the lack of zero-address check in some setters.

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

PermissionlessBasicPoolFactory.sol

PermissionlessBasicPoolFactory.sol:57: mapping (uint => Pool) public pools;
PermissionlessBasicPoolFactory.sol:59: mapping (uint => Metadata) public metadatas;
PermissionlessBasicPoolFactory.sol:61: mapping (uint => uint[]) public taxes;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, the mitigation could be:

struct PoolStruct {
  Pool pool;
  Metadata metadata;
  uint[] tax;
}

And it would be used as a state variable:

mapping(uint256 =>  PoolStruct) pool;

Typos/Spelling mistakes

PROBLEM

There are a few spelling mistakes in the contracts.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

MerkleDropFactory.sol

MerkleDropFactory.sol:76: //effect

MerkleEligibility.sol

MerkleEligibility.sol:11: //effect

MerkleResistor.sol

MerkleResistor.sol:200,201: //effect

MerkleVesting.sol

MerkleVesting.sol:88: //effect
MerkleVesting.sol:170: //effect

PermissionlessBasicPoolFactory.sol

PermissionlessBasicPoolFactory.sol:82: //effect

TOOLS USED

Manual Analysis

MITIGATION

affect is the correct verb to use in these instances.

Uint256 alias

IMPACT

uint is an alias for uint256.

It is better to use uint256: it brings readability and consistency in the code, and it future proofs it in case of any changes to the alias of uint

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

All the contracts in scope, except for VoterID.sol where uint256 is used

TOOLS USED

Manual Analysis

MITIGATION

replace uint with uint256

Unchecked inputs

PROBLEM

There should be a non-zero (address or integer) check in all setters.

SEVERITY

Low

PROOF OF CONCEPT

in MerkleIdentity.sol, setManagement() changes the management address to the one passed as an argument, but there is no zero address check. If it were to be accidentally set to the address zero, functions with the modifier managementOnly could not be called anymore, in particular no new trees could be added to the contract.

TOOLS USED

Manual Analysis

MITIGATION

Add zero-address checks in setters.

Setters should emit an event

PROBLEM

All setters should emit an event, so the Dapps can detect important changes

SEVERITY

Low

PROOF OF CONCEPT

Instances include:

MerkleIdentity.sol

MerkleIdentity.sol:60:setManagement(address newMgmt) 
MerkleIdentity.sol:67:setTreeAdder(address newAdder)
MerkleIdentity.sol:75:setIpfsHash(uint merkleIndex, bytes32 hash)

TOOLS USED

Manual Analysis

MITIGATION

Emit an event in every setter;

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Default value initialization
• Event emitting of local variable
• Prefix increments
• unnecessary computation

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

PROOF OF CONCEPT

Instances include:

MerkleDropFactory.sol

scope: addMerkleTree()

• numTrees is read twice

MerkleDropFactory.sol:59
MerkleDropFactory.sol:61

MerkleEligibility.sol

scope: addGate()

• numGates is read twice

MerkleEligibility.sol:49
MerkleEligibility.sol:50

MerkleResistor.sol

scope: addMerkleTree()

• numTrees is read twice

MerkleResistor.sol:99
MerkleResistor.sol:100

MerkleVesting.sol

scope: addMerkleRoot()

• numTrees is read twice

MerkleVesting.sol:71
MerkleVesting.sol:72

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

MerkleDropFactory.sol

scope: withdraw()

MerkleDropFactory.sol:88: bytes32[] memory proof

MerkleEligibility.sol

scope: isEligible()

MerkleEligibility.sol:70: bytes32[] memory proof

scope: passThruGate()

MerkleEligibility.sol:85: bytes32[] memory proof

MerkleIdentity.sol

scope: withdraw()

MerkleIdentity.sol:116: string memory uri, bytes32[] memory addressProof, bytes32[] memory metadataProof

scope: isEligible()

MerkleIdentity.sol:152: bytes32[] memory proof

scope: verifyMetadata()

MerkleIdentity.sol:163: string memory uri, bytes32[] memory proof

MerkleLib.sol

scope: verifyProof()

MerkleLib.sol:17: bytes32[] memory proof

MerkleResistor.sol

scope: initialize()

MerkleResistor.sol:134: bytes32[] memory proof

MerkleVesting.sol

scope: initialize()

MerkleVesting.sol:104: bytes32[] memory proof

PermissionlessBasicPoolFactory.sol

scope: addPool()

PermissionlessBasicPoolFactory.sol:95: uint[] memory rewardsWeiPerSecondPerToken
PermissionlessBasicPoolFactory.sol:99: address[] memory rewardTokenAddresses

VoterID.sol

scope: createIdentityFor()

VoterID.sol:122: string memory uri

scope: setTokenURI()

VoterID.sol:162: string memory uri

scope: safeTransferFrom()

VoterID.sol:215: bytes memory data

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

FixedPricePassThruGate.sol

FixedPricePassThruGate.sol:48

MerkleDropFactory.sol

MerkleDropFactory.sol:90

MerkleIdentity.sol

MerkleIdentity.sol:124

MerkleResistor.sol

MerkleResistor.sol:179

MerkleVesting.sol

MerkleVesting.sol:153

SpeedBumpPriceGate.sol

SpeedBumpPriceGate.sol:67

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

However, if 1 is negligible compared to the value of the variable, we can omit the increment.

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

MerkleEligibility.sol

MerkleEligibility.sol:35 address _gateMaster

MerkleIdentity.sol

MerkleIdentity.sol:52 address _mgmt

PermissionlessBasicPoolFactory.sol

PermissionlessBasicPoolFactory.sol:75 address _globalBeneficiary, uint _globalTaxPerCapita

VoterID.sol

VoterID.sol:108 address ooner, address minter, string memory nomen, string memory symbowl

TOOLS USED

Manual Analysis

MITIGATION

Hardcode the state variable with its initial value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

FixedPricePassThruGate.sol

FixedPricePassThruGate.sol:48
FixedPricePassThruGate.sol:54

MerkleDropFactory.sol

MerkleDropFactory.sol:77
MerkleDropFactory.sol:90
MerkleDropFactory.sol:92
MerkleDropFactory.sol:98
MerkleDropFactory.sol:107

MerkleEligibility.sol

MerkleEligibility.sol:86
MerkleEligibility.sol:89

MerkleIdentity.sol

MerkleIdentity.sol:97
MerkleIdentity.sol:124
MerkleIdentity.sol:127

MerkleResistor.sol

MerkleResistor.sol:82
MerkleResistor.sol:83
MerkleResistor.sol:121
MerkleResistor.sol:136
MerkleResistor.sol:138
MerkleResistor.sol:144
MerkleResistor.sol:149
MerkleResistor.sol:171
MerkleResistor.sol:175
MerkleResistor.sol:204

MerkleVesting.sol

MerkleVesting.sol:89
MerkleVesting.sol:106
MerkleVesting.sol:113
MerkleVesting.sol:141
MerkleVesting.sol:145
MerkleVesting.sol:147

PermissionlessBasicPoolFactory.sol

PermissionlessBasicPoolFactory.sol:112
PermissionlessBasicPoolFactory.sol:148
PermissionlessBasicPoolFactory.sol:159
PermissionlessBasicPoolFactory.sol:160
PermissionlessBasicPoolFactory.sol:182
PermissionlessBasicPoolFactory.sol:183
PermissionlessBasicPoolFactory.sol:184
PermissionlessBasicPoolFactory.sol:185
PermissionlessBasicPoolFactory.sol:199
PermissionlessBasicPoolFactory.sol:211
PermissionlessBasicPoolFactory.sol:213
PermissionlessBasicPoolFactory.sol:214
PermissionlessBasicPoolFactory.sol:215
PermissionlessBasicPoolFactory.sol:234
PermissionlessBasicPoolFactory.sol:244
PermissionlessBasicPoolFactory.sol:245
PermissionlessBasicPoolFactory.sol:246
PermissionlessBasicPoolFactory.sol:254
PermissionlessBasicPoolFactory.sol:263
PermissionlessBasicPoolFactory.sol:271
PermissionlessBasicPoolFactory.sol:315
PermissionlessBasicPoolFactory.sol:316

SpeedBumpPriceGate.sol

SpeedBumpPriceGate.sol:67
SpeedBumpPriceGate.sol:80

VoterID.sol

VoterID.sol:123
VoterID.sol:124
VoterID.sol:184
VoterID.sol:199
VoterID.sol:217
VoterID.sol:239
VoterID.sol:240
VoterID.sol:262
VoterID.sol:305
VoterID.sol:306
VoterID.sol:437
VoterID.sol:449
VoterID.sol:450

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in MerkleEligibility.sol:

Replace

require(msg.sender == gateMaster, "Only gatemaster may call this.");

with

if (msg.sender != gateMaster) {
		revert IsNotGateMaster(msg.sender);
}

and define the custom error in the contract

error IsNotGateMaster(address _address);

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

MerkleDropFactory.sol

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

MerkleEligibility.sol

MerkleEligibility.sol:31: uint public numGates = 0;

MerkleLib.sol

MerkleLib.sol:22: uint i = 0;

MerkleResistor.sol

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

MerkleVesting.sol

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

PermissionlessBasicPoolFactory.sol

PermissionlessBasicPoolFactory.sol:115: uint i = 0;
PermissionlessBasicPoolFactory.sol:141: uint i = 0;
PermissionlessBasicPoolFactory.sol:168: uint i = 0;
PermissionlessBasicPoolFactory.sol:224: uint i = 0;
PermissionlessBasicPoolFactory.sol:249: uint i = 0;
PermissionlessBasicPoolFactory.sol:266: uint i = 0;

VoterID.sol

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

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Event emitting of local variable

PROBLEM

When emitting an event, using a local variable instead of a storage variable saves gas.

PROOF OF CONCEPT

Instances include:

MerkleDropFactory.sol

MerkleDropFactory.sol:61

MerkleResistor.sol

MerkleResistor.sol:122

MerkleVesting.sol

MerkleVesting.sol:72

TOOLS USED

Manual Analysis

MITIGATION

The storage variable is read multiple times in the function and it is recommended to cache it into memory, then passing the cached variable in the emit statement, as explained in the cache paragraph

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

MerkleLib.sol

MerkleLib.sol:22 i += 1

PermissionlessBasicPoolFactory.sol

PermissionlessBasicPoolFactory.sol:115 i++
PermissionlessBasicPoolFactory.sol:141 i++
PermissionlessBasicPoolFactory.sol:168 i++
PermissionlessBasicPoolFactory.sol:224 i++
PermissionlessBasicPoolFactory.sol:249 i++
PermissionlessBasicPoolFactory.sol:266 i++

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable, and variable += 1 to ++variable.

Unnecessary computation

IMPACT

When emitting an event that includes a new and an old value, it is cheaper in gas to avoid caching the old value in memory. Instead, emit the event, then save the new value in storage.

PROOF OF CONCEPT

Instances include:

VoterID.sol

VoterID.sol:154

TOOLS USED

Manual Analysis

MITIGATION

Replace

address oldOwner = _owner_;
_owner_ = newOwner;
emit OwnerUpdated(oldOwner, newOwner);

with

emit OwnerUpdated(_owner_, newOwner);
_owner_ = newOwner;