Gas Optimizations

[code423n4]

PermissionlessBasicPoolFactory.sol

Argument checks should happen before the logic starts at addPool function

There are some argument checks at addPool function.

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

function addPool (
    ...
) external {
    Pool storage pool = pools[++numPools];
    ...
    pool.taxPerCapita = globalTaxPerCapita;

    require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');

To reduce the gas cost for the user for the failure case, it is worth adding the check at the start of the function.

function addPool (
    ...
) external {
    require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');

    Pool storage pool = pools[++numPools];
    ...
    pool.taxPerCapita = globalTaxPerCapita;

---

Setting 0 is not needed at uint i variable at the for loop

In the for loop, uint i = 0 is used but this is not needed.

for (uint i = 0; i < rewardTokenAddresses.length; i++) {

In this contract, following places have this behavior. https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L115

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

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

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

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

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

The default value of uint is 0, so removing it can reduce the gas cost like this.

for (uint i; i < rewardTokenAddresses.length; i++) {

---

Usage of success boolean flag can be optimized

success boolean flag is used in the for loop. If the transfer fails for any of the token, the operation will be reverted because of require(success, 'Token deposits failed').

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');

If the numbers of loop increase, this can consume gas cost for the failure operations which are not good behavior for users. It can use break in the for loop if success becomes false to reduce the operations of the for loop like this.

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);
    if (!success) {
        break;
    }
    // bookkeeping to make sure pools don't share tokens
    pool.rewardFunding[i] += amount;
}
require(success, 'Token deposits failed');

Following codes have the pattern mentioned above, and worth revisiting.

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L139-L148

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L222-L234

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L248-L254

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L265-L270

---

Unchecked can be used to reduce the gas cost at deposit function

It is certain that pool.maximumDepositWei - pool.totalDepositsWei will not be underflown because of require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached').

https://github.com/code-423n4/2022-05-factorydao/blob/e22a562c01c533b8765229387894cc0cb9bed116/contracts/PermissionlessBasicPoolFactory.sol#L185-L188

require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');
if (pool.totalDepositsWei + amount > pool.maximumDepositWei) {
    amount = pool.maximumDepositWei - pool.totalDepositsWei;
}

This part can be rewritten like this to reduce the gas cost by using unchecked.

require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');
if (pool.totalDepositsWei + amount > pool.maximumDepositWei) {
    unchecked {
        amount = pool.maximumDepositWei - pool.totalDepositsWei;
    }
}

[illuzen]

all duplicates

[gititGoro]

For the success item, why not just require on every transfer?

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

Note to sponsor: the existing code assumes the token conforms to the ERC20 standard and returns a boolean. If you'd like to support tokens that don't, consider using a safe transfer approach such as this pattern used in Uniswap V2 (https://github.com/Uniswap/v2-core/blob/8b82b04a0b9e696c0e83f8b2f00e5d7be6888c79/contracts/UniswapV2Pair.sol#L44)