QA Report

[code423n4]

QA (LOW RISK & NON-CRITICAL)

• The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success.
  Furthermore, some tokens (like USDT) don't correctly implement the ERC20 standard and don't return a boolean.Return values of the transfers are not checked in below LOC's.

IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal); MerkleVesting.sol#L173
success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount); PermissionlessBasicPoolFactory.sol#L144
success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount); PermissionlessBasicPoolFactory.sol#L230
success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei); PermissionlessBasicPoolFactory.sol#L233
success = success && IERC20(pool.rewardTokens[i]).transfer(pool.excessBeneficiary, rewards); PermissionlessBasicPoolFactory.sol#L252
success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary, tax); PermissionlessBasicPoolFactory.sol#L269

• .transfer is used for transferring ether. It is no longer recommended as recipients with custom fallback functions (smart contracts) will not be able to handle that. Reference

• FixedPricePassThruGate.sol doesn't have function to receive ether/funds.

• FixedPricePassThruGate.sol, addGate() function doesn't check address(0) for _beneficiary parameter which can lead the funds to burn.

• At FixedPricePassThruGate.sol, passThruGate() function did not explicitly state the input variables and NatSpec is not sufficient for this function;

   function passThruGate(uint index, address) override external payable {

• At FixedPricePassThruGate.sol, anyone can add arbitrary amount of gates including attacker's address as beneficiary by addGate() function. Thus, this leads to creating other attack surfaces like phishing. The contract should have strict control of creating gates like no duplicate gates should be allowed with same beneficiary address.

• At FixedPricePassThruGate.sol, data variable is not used and the best practice is to remove unused variables
  Reference

 (bool sent, bytes memory data) = gate.beneficiary.call{value: gate.ethCost}("");

• At MerkleEligibility.sol, there is no address(0) check at the constructor function for _gateMaster

• At MerkleEligibility.sol, named return and returned variable is not matching;

    function addGate(bytes32 merkleRoot, uint maxWithdrawalsAddress, uint maxWithdrawalsTotal) external returns (uint index) {
        // increment the number of roots
        numGates += 1;

        gates[numGates] = Gate(merkleRoot, maxWithdrawalsAddress, maxWithdrawalsTotal, 0);
        return numGates;
    }

• At MerkleEligibility.sol, isEligible() function, to reach the boundry, the logic should be;

bool countValid = timesWithdrawn[index][recipient] <= gate.maxWithdrawalsAddress;

instead of;

bool countValid = timesWithdrawn[index][recipient] < gate.maxWithdrawalsAddress;

• At MerkleIdentity.sol, constructor function, there is no address(0) check for _mgmt parameter and for setManagement() function, no address(0) check for newMgmt. It's recommended that the setManagement function is not handled in one step. The team might consider to use Ownable.sol of Open Zeppelin.

• At MerkleIdentity.sol, for setTreeAdder() function, no address(0) check for newAdder. It's recommended that the setTreeAdder function is not handled in one step. The team might consider to use Ownable.sol of Open Zeppelin.

• At MerkleIdentity.sol, setIpfsHash() function, there is no 0 value check for both the parameters.

• At MerkleIdentity.sol, addMerkleTree() function, there is no 0 value and address(0) check for the parameters.

• At PermissionlessBasicPoolFactory.sol, costly operations used inside a loop which might lead to an out-of-gas.
  At addPool();

for (uint i = 0; i < rewardTokenAddresses.length; i++) {
           pool.rewardTokens.push(rewardTokenAddresses[i]);
           pool.rewardsWeiClaimed.push(0);
           pool.rewardFunding.push(0);
           taxes[numPools].push(0);
       }

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

At withdraw();

for (uint i = 0; i < rewards.length; i++) {
            pool.rewardsWeiClaimed[i] += rewards[i];
            pool.rewardFunding[i] -= rewards[i];
            uint tax = (pool.taxPerCapita * rewards[i]) / 1000;
            uint transferAmount = rewards[i] - tax;
            taxes[poolId][i] += tax;
            success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);
        }

function withdrawTaxes(uint poolId) external {
        Pool storage pool = pools[poolId];
        require(pool.id == poolId, 'Uninitialized pool');

        bool success = true;
        for (uint i = 0; i < pool.rewardTokens.length; i++) {
            uint tax = taxes[poolId][i];
            taxes[poolId][i] = 0;
            success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary, tax);
        }
        require(success, 'Token transfer failed');
    }

• At PermissionlessBasicPoolFactory.sol, there has been calls inside a loop which may end up wit DoS.
  Reference

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

function withdraw(uint poolId, uint receiptId) external {
        Pool storage pool = pools[poolId];
        require(pool.id == poolId, 'Uninitialized pool');
        Receipt storage receipt = pool.receipts[receiptId];
        require(receipt.id == receiptId, 'Can only withdraw real receipts');
        require(receipt.owner == msg.sender || block.timestamp > pool.endTime, 'Can only withdraw your own deposit');
        require(receipt.timeWithdrawn == 0, 'Can only withdraw once per receipt');

        // close re-entry gate
        receipt.timeWithdrawn = block.timestamp;

        uint[] memory rewards = getRewards(poolId, receiptId);
        pool.totalDepositsWei -= receipt.amountDepositedWei;
        bool success = true;

        for (uint i = 0; i < rewards.length; i++) {
            pool.rewardsWeiClaimed[i] += rewards[i];
            pool.rewardFunding[i] -= rewards[i];
            uint tax = (pool.taxPerCapita * rewards[i]) / 1000;
            uint transferAmount = rewards[i] - tax;
            taxes[poolId][i] += tax;
            success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);
        }

function withdrawExcessRewards(uint poolId) external {
        Pool storage pool = pools[poolId];
        require(pool.id == poolId, 'Uninitialized pool');
        require(pool.totalDepositsWei == 0, 'Cannot withdraw until all deposits are withdrawn');
        require(block.timestamp > pool.endTime, 'Contract must reach maturity');

        bool success = true;
        for (uint i = 0; i < pool.rewardTokens.length; i++) {
            uint rewards = pool.rewardFunding[i];
            pool.rewardFunding[i] = 0;
            success = success && IERC20(pool.rewardTokens[i]).transfer(pool.excessBeneficiary, rewards);
        }
        require(success, 'Token transfer failed');
        emit ExcessRewardsWithdrawn(poolId);
    }

unction withdrawTaxes(uint poolId) external {
        Pool storage pool = pools[poolId];
        require(pool.id == poolId, 'Uninitialized pool');

        bool success = true;
        for (uint i = 0; i < pool.rewardTokens.length; i++) {
            uint tax = taxes[poolId][i];
            taxes[poolId][i] = 0;
            success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary, tax);
        }
        require(success, 'Token transfer failed');
    }

[illuzen]

all duplicates, and the .transfers are for tokens, not ether