FactoryDAO contest - ellahi's results

Low

1. Missing checks for address(0) when assigning values to address state variables.

Impact

Setters of address type parameters should include a zero-address check otherwise contract functionality may become inaccessible or tokens burnt forever.

Proof of Concept

Check @audit tags.

PermissionlessBasicPoolFactory.sol#L75-L78

constructor(address _globalBeneficiary, uint _globalTaxPerCapita) {
    globalBeneficiary = _globalBeneficiary; // @audit missing address(0) check.
    globalTaxPerCapita = _globalTaxPerCapita;
}

MerkleIdentity.sol#L52-L69

constructor(address _mgmt) {
    management = _mgmt; // @audit missing address(0) check.
    treeAdder = _mgmt;
}
...
function setManagement(address newMgmt) external managementOnly {
    management = newMgmt; // @audit missing address(0) check.
}
...
function setTreeAdder(address newAdder) external managementOnly {
    treeAdder = newAdder; // @audit missing address(0) check.
}

VoterID.sol#L108-L114

constructor(address ooner, address minter, string memory nomen, string memory symbowl) {
    _owner_ = ooner; // @audit missing address(0) check.
    // we set it here with no resetting allowed so we cannot commit to NFTs and then reset
    _minter = minter; // @audit missing address(0) check.
    _name = nomen;
    _symbol = symbowl;
}

VoterID.sol#L151-L155

function setOwner(address newOwner) external ownerOnly {
    address oldOwner = _owner_; // @audit missing address(0) check.
    _owner_ = newOwner;
    emit OwnerUpdated(oldOwner, newOwner);
}

Recommendation

Add address(0) checks.

2. Incorrect documentation.

Impact

The @dev comment for PermissionlessBasicPoolFactory.sol::withdraw() states that only the receipt owner can call this function, but the assertion says otherwise.

Proof of Concept

...
/// @dev Only receipt owner may call this function
...
function withdraw(uint poolId, uint receiptId) external {
    ...
    require(receipt.owner == msg.sender || block.timestamp > pool.endTime, 'Can only withdraw your own deposit');
    ...

If block.timestamp > pool.endTime, then this function is callable by anyone and not just the owner.

Recommendation

Fix comments

3. Unsafe ERC20 Operation

Impact

The return value of an external transfer/transferFrom call is not checked.

Proof of Concept

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

Recommendation

Use SafeERC20, or ensure that the transfer/transferFrom return value is checked.

4. Mismatch in Pragma Version accross contracts.

Impact

PermissionlessBasicPoolFactory.sol uses pragma solidity 0.8.12 whereas every other contract uses pragma solidity 0.8.9

Recommendation

Use same compiler version.

Non-Critical

1. Declare uint as uint256.

Recommendation

To favor explicitness, all instances of uint should be declared as uint256.

Tools used

Manual

Gas

1. Cache Array Length Outside of Loop.

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

  MerkleLib.sol::22 => for (uint i = 0; i < proof.length; i += 1) {
  PermissionlessBasicPoolFactory.sol::115 => for (uint i = 0; i < rewardTokenAddresses.length; i++) {
  PermissionlessBasicPoolFactory.sol::141 => for (uint i = 0; i < pool.rewardFunding.length; i++) {
  PermissionlessBasicPoolFactory.sol::168 => for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
  PermissionlessBasicPoolFactory.sol::224 => for (uint i = 0; i < rewards.length; i++) {
  PermissionlessBasicPoolFactory.sol::249 => for (uint i = 0; i < pool.rewardTokens.length; i++) {
  PermissionlessBasicPoolFactory.sol::266 => for (uint i = 0; i < pool.rewardTokens.length; i++) {

Recommendation

Store the array’s length in a variable before the for-loop.

2. Use Custom Errors instead of Revert Strings.

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)

Proof of Concept

Throughout the codebase.

Recommendation

Use custom errors instead of revert strings.

3. ++i costs less gas compared to i++ or i += 1

Impact

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

Proof of Concept

  MerkleLib.sol::22 => for (uint i = 0; i < proof.length; i += 1) {
  PermissionlessBasicPoolFactory.sol::115 => for (uint i = 0; i < rewardTokenAddresses.length; i++) {
  PermissionlessBasicPoolFactory.sol::141 => for (uint i = 0; i < pool.rewardFunding.length; i++) {
  PermissionlessBasicPoolFactory.sol::168 => for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
  PermissionlessBasicPoolFactory.sol::224 => for (uint i = 0; i < rewards.length; i++) {
  PermissionlessBasicPoolFactory.sol::249 => for (uint i = 0; i < pool.rewardTokens.length; i++) {
  PermissionlessBasicPoolFactory.sol::266 => for (uint i = 0; i < pool.rewardTokens.length; i++) {

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

4. No need to initialize variables with default values

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

  MerkleLib.sol::22 => for (uint i = 0; i < proof.length; i += 1) {
  MerkleResistor.sol::176 => uint currentWithdrawal = 0;
  MerkleVesting.sol::150 => uint currentWithdrawal = 0;
  PermissionlessBasicPoolFactory.sol::115 => for (uint i = 0; i < rewardTokenAddresses.length; i++) {
  PermissionlessBasicPoolFactory.sol::141 => for (uint i = 0; i < pool.rewardFunding.length; i++) {
  PermissionlessBasicPoolFactory.sol::168 => for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
  PermissionlessBasicPoolFactory.sol::224 => for (uint i = 0; i < rewards.length; i++) {
  PermissionlessBasicPoolFactory.sol::249 => for (uint i = 0; i < pool.rewardTokens.length; i++) {
  PermissionlessBasicPoolFactory.sol::266 => for (uint i = 0; i < pool.rewardTokens.length; i++) {
  VoterID.sol::69 => uint public numIdentities = 0;

Recommendation

Remove explicit zero initialization.

Tools used

c4udit, manual, slither.