FactoryDAO contest - gzeon's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L46-L56

Vulnerability details

Impact

In passThruGate function, msg.value is checked to be greater than the required cost, but the excess amount is not returned to the sender.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/FixedPricePassThruGate.sol#L46-L56

    function passThruGate(uint index, address) override external payable {
        Gate memory gate = gates[index];
        require(msg.value >= gate.ethCost, 'Please send more ETH');

        // pass thru ether
        if (msg.value > 0) {
            // use .call so we can send to contracts, for example gnosis safe, re-entrance is not a threat here
            (bool sent, bytes memory data) = gate.beneficiary.call{value: gate.ethCost}("");
            require(sent, 'ETH transfer failed');
        }
    }

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/SpeedBumpPriceGate.sol#L64-L82

    function passThruGate(uint index, address) override external payable {
        uint price = getCost(index);
        require(msg.value >= price, 'Please send more ETH');

        // bump up the price
        Gate storage gate = gates[index];
        // multiply by the price increase factor
        gate.lastPrice = (price * gate.priceIncreaseFactor) / gate.priceIncreaseDenominator;
        // move up the reference
        gate.lastPurchaseBlock = block.number;

        // pass thru the ether
        if (msg.value > 0) {
            // use .call so we can send to contracts, for example gnosis safe, re-entrance is not a threat here
            (bool sent, bytes memory data) = gate.beneficiary.call{value: msg.value}("");
            require(sent, 'ETH transfer failed');
        }
    }

Recommended Mitigation Steps

Return excess ETH to sender or make the require strict equal.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L173-L174

Vulnerability details

Impact

Some ERC20.transfer does not revert upon failure, MerkleVesting contract does not check the return value of the ERC20.transfer and user will not be able to claim again due to currentWithdrawal is subtracted from tranche.currentCoins.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L173-L174

        IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L161-L162

        tranche.currentCoins -= currentWithdrawal;

Recommended Mitigation Steps

Use safeTransfer or otherwise check the return value of ERC20.transfer()

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L117-L118

Vulnerability details

Impact

coinsPerSecond is totalCoins(in token's decimals) divided by the length of vesting period in seconds. For low decimal token such as USDC, coinsPerSecond might have significant precision loss that will make user received less than the amount entitled.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L117-L118

        uint coinsPerSecond = totalCoins / (endTime - startTime);

For example if 0.01 WBTC is vested over 1 year coinsPerSecond = 0.01 * 10e8 / (365*86400) = 0 which the user will not be able to claim any of the balance

Recommended Mitigation Steps

Multiply coinsPerSecond by for example 10e18

Low

Solidity version mismatch

PermissionlessBasicPoolFactory use 0.8.12 while other file use 0.8.9 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L3-L4

pragma solidity 0.8.12;

Lack events on critical parameters update

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

    function setGlobalTax(uint newTaxPerCapita) external {
        require(msg.sender == globalBeneficiary, 'Only globalBeneficiary can set tax');
        require(newTaxPerCapita < 1000, 'Tax too high');
        globalTaxPerCapita = newTaxPerCapita;
    }

Non-Critical

management and treeAdder can be set to address(0)

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleIdentity.sol#L52-L69

    constructor(address _mgmt) {
        management = _mgmt;
        treeAdder = _mgmt;
    }

    /// @notice Change the management key
    /// @dev Only the current management key can change this
    /// @param newMgmt the new management key
    function setManagement(address newMgmt) external managementOnly {
        management = newMgmt;
    }

    /// @notice Change the treeAdder key
    /// @dev Only the current management key can call this
    /// @param newAdder new addres that will be able to add trees, old address will not be able to
    function setTreeAdder(address newAdder) external managementOnly {
        treeAdder = newAdder;
    }

Mul-after-Div might lower precision

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L246-L250

            uint paymentSlope = (maxTotalPayments - minTotalPayments) * PRECISION / (tree.maxEndTime - tree.minEndTime);

            // y = mx + b = paymentSlope * (x - x0) + y0
            // divide by precision factor here since we have completed the rounding error sensitive operations
            totalCoins = (paymentSlope * (vestingTime - tree.minEndTime) / PRECISION) + minTotalPayments;

Comparison should be inclusive

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

        require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');

++numGates instead of numGates += 1

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleEligibility.sol#L47-L48

        numGates += 1;

Better struct packing

There are some instance that there is an id field and some address field. Normally id won't need full 256 bit and can be packed into the same slot as the address fields. https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L15-L39

    struct Receipt {
        uint id;   // primary key
        uint amountDepositedWei;  // amount of tokens originally deposited
        uint timeDeposited;  // the time the deposit was made
        uint timeWithdrawn;  // the time the deposit was withdrawn, or 0 if not withdrawn yet
        address owner;  // the owner of the deposit
    }

    // this represents a single staking pool with >= 1 reward tokens
    struct Pool {
        uint id; // primary key
        uint[] rewardsWeiPerSecondPerToken; // array of reward rates, this number gets multiplied by time and tokens (not wei) to determine rewards
        uint[] rewardsWeiClaimed;  // bookkeeping of how many rewards have been paid out for each token
        uint[] rewardFunding;  // bookkeeping of how many rewards have been supplied for each token
        uint maximumDepositWei;  // the size of the pool, maximum sum of all deposits
        uint totalDepositsWei;  // current sum of all deposits
        uint numReceipts;  // number of receipts issued
        uint startTime;  // the time that the pool begins
        uint endTime;    // time that the pool ends
        uint taxPerCapita;  // portion of rewards that go to the contract creator
        address depositToken;  // token that user deposits (stakes)
        address excessBeneficiary;  // address that is able to reclaim unused rewards
        address[] rewardTokens;  // array of token contract addresses that stakers will receive as rewards
        mapping (uint => Receipt) receipts;  // mapping of receipt ids to receipt structs
    }

Unchecked safe math

The increment of numTrees and numPools will never overflow an uint256, we can wrap them in unchecked block.

contracts/PermissionlessBasicPoolFactory.sol:103:        Pool storage pool = pools[++numPools];
contracts/MerkleResistor.sol:86:        merkleTrees[++numTrees] = MerkleTree(
contracts/MerkleVesting.sol:64:        merkleTrees[++numTrees] = MerkleTree(
contracts/FixedPricePassThruGate.sol:30:        Gate storage gate = gates[++numGates];
contracts/MerkleIdentity.sol:98:        MerkleTree storage tree = merkleTrees[++numTrees];
contracts/SpeedBumpPriceGate.sol:39:        Gate storage gate = gates[++numGates];
contracts/MerkleDropFactory.sol:51:        merkleTrees[++numTrees] = MerkleTree(

Mark variable as immutable

Some variable can be marked as immutable to save gas https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L51-L54

    address public globalBeneficiary;

Avoid unnecessary SLOADS

pool.endTime can be calculated without using pool.startTime https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L106-L107

        pool.startTime = startTime > block.timestamp ? startTime : block.timestamp;
        pool.endTime = (startTime > block.timestamp ? startTime : block.timestamp) + (programLengthDays * 1 days);

Earlier revert

Instead of && the success flag in each iteration, we can have require(success, 'Token deposits failed'); inside the for loop. https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L137-L149

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

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

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

        success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);
        require(success, 'Token transfer failed');

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

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

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

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

For loop optimization

for(uint256 i; i < length; ){
...
unchecked{ ++i; }
}

Use custom errors

Solidity ^0.8.4 allow the use of custom errors to optimize gas usage. https://blog.soliditylang.org/2021/04/21/custom-errors/

Non-zero check of pool.id is sufficient

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

        require(pool.id == poolId, 'Uninitialized pool');