FactoryDAO contest - TerrierLover's results

Interfaces

IERC20.sol, IERC721.sol, and IERC721Receiver.sol seem to have very similar implementations with openzeppelin's counterparts.

• IERC20: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/IERC20.sol
• IERC721: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721.sol
• IERC721Receiver: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721Receiver.sol

It is worth using the latest versions if possible.

---

IERC721.sol

Lack of implementation of IERC165

IERC721.sol in the FactoryDAO project does not implement IERC165. https://github.com/code-423n4/2022-05-factorydao/blob/main/interfaces/IERC721.sol#L8

Here is an implementation of the IERC721 of openzeppelin.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/IERC721.sol

interface IERC721 is IERC165 {

It may be worth adding the support of the interface of ERC165 in this project. Reference

---

Styles of functions' arguments are not consistent

Arguments in the functions used in IERC721 have type variableName. However, following two parts are exceptions.

https://github.com/code-423n4/2022-05-factorydao/blob/main/interfaces/IERC721.sol#L104

function setApprovalForAll(address operator, bool _approved) external;

https://github.com/code-423n4/2022-05-factorydao/blob/main/interfaces/IERC721.sol#L111

function isApprovedForAll(address, address) external view returns (bool);

If this peoject keeps using the copy of IERC721, it is worth fixing the above style inconsistency as follows.

function setApprovalForAll(address operator, bool approved) external;

function isApprovedForAll(address owner, address operator) external view returns (bool);

---

PermissionlessBasicPoolFactory.sol

Consider adding address(0) check on _globalBeneficiary at constructor

_globalBeneficiary variable does not have any address(0) check.

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

constructor(address _globalBeneficiary, uint _globalTaxPerCapita) {
    globalBeneficiary = _globalBeneficiary;
    globalTaxPerCapita = _globalTaxPerCapita;
}

There is no way to update the state variable globalBeneficiary, so it is worth considering adding address(0) check.

constructor(address _globalBeneficiary, uint _globalTaxPerCapita) {
    require(_globalBeneficiary != address(0), "...");
    globalBeneficiary = _globalBeneficiary;
    globalTaxPerCapita = _globalTaxPerCapita;
}

---

rewardsWeiPerSecondPerToken and rewardTokenAddresses can be empty array at addPool

addPool receives rewardsWeiPerSecondPerToken and rewardTokenAddresses array.

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

function addPool (
    uint startTime,
    uint maxDeposit,
    uint[] memory rewardsWeiPerSecondPerToken,
    uint programLengthDays,
    address depositTokenAddress,
    address excessBeneficiary,
    address[] memory rewardTokenAddresses,
    bytes32 ipfsHash,
    bytes32 name
) external {

It is possible that user can add empty array on rewardsWeiPerSecondPerToken and rewardTokenAddresses. In this case, the pool does not do anything. If this is not expected, it is worth adding the empty array check at addPool function.

function addPool (
    uint startTime,
    uint maxDeposit,
    uint[] memory rewardsWeiPerSecondPerToken,
    uint programLengthDays,
    address depositTokenAddress,
    address excessBeneficiary,
    address[] memory rewardTokenAddresses,
    bytes32 ipfsHash,
    bytes32 name
) external {
    require(rewardsWeiPerSecondPerToken.length != 0, "rewardsWeiPerSecondPerToken cannot be empty");

---

withdrawTaxes function does not have event

withdrawTaxes function does not have event emitted while withdrawExcessRewards function does have ExcessRewardsWithdrawn emission.

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

For the consistency, withdrawTaxes should have the event emitted.

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;
    }
}

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;
}