FactoryDAO contest - GimelSec's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/FixedPricePassThruGate.sol#L48 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/SpeedBumpPriceGate.sol#L67

Vulnerability details

Impact

When a user send ETH via the withdraw() of MerkleIdentity, It will call IPriceGate(tree.priceGateAddress).passThruGate{value: msg.value}(tree.priceIndex, msg.sender);. passThruGate() checks whether msg.value >= gate.ethCost holds. If a user accidentally sends more ETH than he has to, the contract just accepts it.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/FixedPricePassThruGate.sol#L48

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

Tools Used

vim

Recommended Mitigation Steps

Change >= to ==

or

Add a refund mechanism like

    function passThruGate(uint index, address sender) 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');
        }

        // refund
        if (msg.value > price) {
            payable(sender).transfer(msg.value - price);
        }
    }

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L146 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleDropFactory.sol#L73 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleVesting.sol#L85 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L112

Vulnerability details

Impact

Some tokens have a fee on each transfer. The protocol doesn’t handle the fee when transferring ERC20 tokens, leading to the inconsistent amount of token actually received in the contract.

The impact is that a pool will not afford enough rewards, but the amount value is greater than the amount of token actually received, resulting in the overuse of reward tokens from other pools when users call withdraw.

Also, when the total amount of tokens in the pool is insufficient, withdrawExcessRewards will also be unable to claim by excessBeneficiary.

Proof of Concept

In fundPool:

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

pool.rewardFunding should not store amount directly. Some ERC20 transfers will charge a fee, causing the amount to not match the actual number of tokens received.

Same scenario in MerkleDropFactory, MerkleVesting and MerkleResistor contracts.

Tools Used

vim

Recommended Mitigation Steps

Use balanceAfter - balanceBefore rather than using amount directly:

function retrieveTokens(address sender, uint256 amount) public {
  uint256 balanceBefore = deflationaryToken.balanceOf(address(this));
  deflationaryToken.transferFrom(sender, address(this), amount);
  uint256 balanceAfter = deflationaryToken.balanceOf(address(this));
  amount = balanceAfter.sub(balanceBefore);
  totalTokenTransferred += amount;
}

Token Transfer Calculation Accuracy in this article: https://blog.chain.link/defi-security-best-practices/

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L259 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L264

Vulnerability details

Impact

If a user tries to claim a few totalCoins with a long vestingTime, this user will call the initialize function failed, and can not withdraw funds.

Proof of Concept

In MerkleResistor.sol L259:

uint coinsPerSecond = (totalCoins * (uint(100) - tree.pctUpFront)) / (vestingTime * 100);

If a user try to call initialize function with few totalCoins (which is limited by maxTotalPayments) and a long vestingTime (which is limited the lower bond by tree.minEndTime), the user may get coinsPerSecond to 0.

If coinsPerSecond is 0, it will break L264 due to division by zero:

uint startTime = block.timestamp + vestingTime - (totalCoins / coinsPerSecond);

The user will not be able to initialize and withdraw funds.

Tools Used

vim

Recommended Mitigation Steps

Save totalCoins and coinsPerSecond with PRECISION to calculate startTime to avoid division by zero.

Or check coinsPerSecond:

    if (coinsPerSecond == 0) {
        startTime = block.timestamp;
    }

Summary

We list 1 low-critical finding and 1 non-critical finding:

• (Low) MerkleResistor.sol verifyVestingSchedule will be broken when minEndTime == 0 and someone claim vestingTime = 0
• (Non) It’s better to emit an events In MerkleIdentity.sol setter functions

(Low) MerkleResistor.sol verifyVestingSchedule will be broken when minEndTime == 0 and someone claim vestingTime = 0

Impact

If the minEndTime is set to 0 and a user tries to claim vestingTime to 0, the user will be reverted due to division by zero.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleResistor.sol#L259

        uint coinsPerSecond = (totalCoins * (uint(100) - tree.pctUpFront)) / (vestingTime * 100);

Tools Used

vim

Recommended Mitigation Steps

Check minEndTime != 0 in addMerkleTree:

        require(minEndTime != 0, 'minEndTime should not be 0');

(Non) It’s better to emit an events In MerkleIdentity.sol setter functions

Impact

It’s better to have events in setter functions.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L60-L62 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L67-L69 https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/MerkleIdentity.sol#L75-L78

Tools Used

vim

Recommended Mitigation Steps

Add events for setter functions.

Use ++numGates instead of numGates += 1

Use ++numGates instead of numGates += 1 to save gas

Proof of Concept

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

Recommendation

Use ++numGates instead of numGates += 1

Fetch pool.numReceipts into memory to save gas

If storage is accessed more than once, it should be fetch into memory to save gas. pool.numReceipts is accessed more than once in deposit()

Proof of Concept

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

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

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

Recommendation

Fetch pool.numReceipts into memory

uint numReceipts = pool.numReceipts;