FactoryDAO contest - CertoraInc's results

Lines of code

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

Vulnerability details

Impact

The ERC20 functions transfer and transferFrom are assumed to return a boolean value, but this is not the case all the time. There are some tokens that their transfer functions don't return anything, and the system won't support them (it'll revert when trying to transfer them because the function that returns boolean doesn't exist)

I added a link to a specific usage, but you use transfer and transferFrom in many more places in the code.

Tools Used

Remix and VSCode

Recommended Mitigation Steps

Use safeTransfer and safeTransferFrom function instead of the regular transfer and transferFrom functions

Lines of code

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

Vulnerability details

Impact

The system won't treat fee on transfer tokens correctly, and some actions can be executed wrongly.

For example, the rewards contract PermissionlessBasicPoolFactory will calculate the deposited amounts in a wrong way, because it uses the amount parameter and not the actual amount of tokens that the contract received.

There are more places where fee on transfer tokens can fail, like the vesting and resistor contracts and more.

Tools Used

Remix and VS Code

Recommended Mitigation Steps

Calculate the deposit amount by the difference between the balance after and before the transfer instead of using the amount parameter

Gas optimizations

• Use prefix incrementation instead of postfix incrementation in line 190 in the PermissionlessBasicPoolFactory contract (++pool.numReceipts instead of pool.numReceipts++). It'll save gas and will have the same effect as pool.numReceipts++.

• The for loop in the fundPool function of the PermissionlessBasicPoolFactory contract can be optimized in several ways:

  function fundPool(uint poolId) internal {
      Pool storage pool = pools[poolId];
      bool success = true;
      uint amount;
      for (uint i = 0; i < pool.rewardFunding.length; i++) {
          amount = getMaximumRewards(poolId, i);
          // transfer the tokens from pool-creator to this contract
          success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount);
          // bookkeeping to make sure pools don't share tokens
          pool.rewardFunding[i] += amount;
      }
      require(success, 'Token deposits failed');
  }

  1. Variables in solidity are already initialized to their default value, and initializing them to the same value actually costs more gas. So for example in the loop above, the code can be optimized using uint i; instead of uint i = 0;.
  2. Use ++i instead of i++ to save some gas spent in every iteration.
  3. Use unchecked when incrementing the loop index (it's not likely to overflow because it'll go out of gas first)
  4. Save the array length in a local variable before the loop instead of accessing it in every iteration and using unnecessary SLOADs.
  5. Simply revert instead of keep going if one transfer has gone wrong

  After the changes, the function will look something like this:

  function fundPool(uint poolId) internal {
      Pool storage pool = pools[poolId];
      uint amount;
      uint len = pool.rewardFunding.length;
      for (uint i; i < len; ) {
          amount = getMaximumRewards(poolId, i);
          // transfer the tokens from pool-creator to this contract
          if (!IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount)) {
              revert('Token deposits failed');
          }
          // bookkeeping to make sure pools don't share tokens
          pool.rewardFunding[i] += amount;
  
          unchecked { ++i; }
      }
  }

  There are multiple for loops in the code that can be optimized using the same optimizations, it'll cost less gas if you'll use these optimizations.

• Save the pool id in the addPool function of the PermissionlessBasicPoolFactory contract to prevent multiple SLOADs.

  The new implementation will look something like this:

  function addPool (
      uint startTime,
      uint maxDeposit,
      uint[] memory rewardsWeiPerSecondPerToken,
      uint programLengthDays,
      address depositTokenAddress,
      address excessBeneficiary,
      address[] memory rewardTokenAddresses,
      bytes32 ipfsHash,
      bytes32 name
  ) external {
      uint256 poolId = ++numPools;
      Pool storage pool = pools[poolId];
      pool.id = poolId;
      pool.rewardsWeiPerSecondPerToken = rewardsWeiPerSecondPerToken;
      pool.startTime = startTime > block.timestamp ? startTime : block.timestamp;
      pool.endTime = pool.startTime + (programLengthDays * 1 days);
      pool.depositToken = depositTokenAddress;
      pool.excessBeneficiary = excessBeneficiary;
      pool.taxPerCapita = globalTaxPerCapita;
  
      require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');
  
      // fill out the arrays with zeros
      for (uint i = 0; i < rewardTokenAddresses.length; i++) {
          pool.rewardTokens.push(rewardTokenAddresses[i]);
          pool.rewardsWeiClaimed.push(0);
          pool.rewardFunding.push(0);
          taxes[poolId].push(0);
      }
      pool.maximumDepositWei = maxDeposit;
  
      // this must be after pool initialization above
      fundPool(poolId);
  
      {
          Metadata storage metadata = metadatas[poolId];
          metadata.ipfsHash = ipfsHash;
          metadata.name = name;
      }
      emit PoolAdded(poolId, name, depositTokenAddress);
  }