code423n4 commented 2 years ago

Interfaces

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

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.

illuzen commented 2 years ago

all duplicates

gititGoro commented 2 years ago

For the first item, it's common practice for devs to re-write an interface locally to lock down features and strip out what isn't needed. Openzeppelin often fills up their contracts with code related to their gas savings network that, in the course of a dapp that is not interested in that, is simply bloat which reduces the final deploy size possible for any downstream contracts.

code-423n4 / 2022-05-factorydao-findings

QA Report #280