FactoryDAO contest - hyh's results

Judge has assessed an item in Issue #285 as High risk. The relevant finding follows:

1. Excess ether sent to FixedPricePassThruGate is lost (low)

passThruGate() redirects to a beneficiary only gate.ethCost, requiring that msg.value >= gate.ethCost. As there are no other ways to access native tokens held by this contract, the cumulative excess value, a sum of msg.value - gate.ethCost, will be permanently frozen within the contract.

Proof of Concept

Only gate.ethCost is now forwarded:

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

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

Recommended Mitigation Steps

Pass-though the whole msg.value as the excess is not used:

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

Lines of code

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

Vulnerability details

The tree.tokenAddress ERC20 can be a token that doesn't revert on failure (i.e. non-malicious, valid ERC20 with such specifics), while transfer call without a result check is used for claiming funds.

The transfer is a part of the only way for a merkle-drop recipient to withdraw her tokens, so a situation when transfer didn't went through, but withdraw() didn't reverted means that recipient funds are frozen within the contract as accounting is updated as if the funds are sent.

As this is user permanent fund freeze case with the external assumptions, setting the severity to medium.

Proof of Concept

Unsafe transfer is used in withdraw():

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

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

Some tokens do not revert on a failed transfer, returning false instead, which technically doesn't break ERC20 standard:

https://github.com/d-xo/weird-erc20#no-revert-on-failure

As withdraw() updates the accounting, the funds can be permanently frozen for a user:

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

  // decrease allocation of coins
  tranche.currentCoins -= currentWithdrawal;
  // this makes sure coins don't get double withdrawn
  tranche.lastWithdrawalTime = block.timestamp;

  // update the tree balance so trees can't take each other's tokens
  MerkleTree storage tree = merkleTrees[treeIndex];
  tree.tokenBalance -= currentWithdrawal;

Recommended Mitigation Steps

Consider requiring tree.tokenAddress transfer success:

require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), "Token transfer failed")

Lines of code

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

Vulnerability details

Griefing attack is possible if pool deposit token is a fee on transfer ERC20 as {deposit, withdraw} atomic call sequence is allowed with pre-fee token quantity being accounted as deposit amount.

Suppose F is a fee on transfer token and the pool with F as deposit token was created (as there are no restrictions and the system didn't declared the absence of support for such tokens).

A malicious user can deposit, then withdraw same amount of F she posses. The attacker incurs a transaction fee plus a transfer fee. The pool will pay a transfer fee as withdraw() will send the exactly same amount as user stated in deposit(), while actually received amount was less. An attacker can repeat this many times over, destroying up to all F funds on the pools balance.

There is no economic benefit for the attacker, but in some scenarios there can be incentives to destroy pool's balance even bearing the same cost on the attacker's side.

This is a griefing attack that have pool principal funds destruction impact with the external assumptions, so setting the severity to medium.

Proof of Concept

deposit() accounts exactly the amount requested as amount deposited (while it can be amount * (1 - fee)):

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

        Receipt storage receipt = pool.receipts[pool.numReceipts];
        receipt.id = pool.numReceipts;
        receipt.amountDepositedWei = amount;
        receipt.timeDeposited = block.timestamp;
        receipt.owner = msg.sender;

        bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);

withdraw() allows for immediate withdrawal of the same receipt.amountDepositedWei:

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

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

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

success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);

Some ERC20 tokens incur/can incur the fee on transfers:

https://github.com/d-xo/weird-erc20#fee-on-transfer

Recommended Mitigation Steps

If there are plans to support fee on transfer tokens then consider using balance difference approach for the deposit amount detection.

Otherwise, state in the documentation that such tokens aren't supported and warn on the corresponding attack vectors.

Lines of code

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

Vulnerability details

Reward tokens that do not allow for zero amount transfers can prevent user pool exit.

Now it is required that all reward amounts be successfully transferred to a receipt owner and the reward token amount isn't checked in the process.

If withdraw was called at the moment when some reward amount is zero (because either zero time passed or zero slope is set), the withdraw() will revert.

Say once such reward token is there (say with no malicious intent, as it's just a specifics of some valid tokens), user cannot withdraw immediately after deposit as no rewards accrued yet and this token transfer will revert the whole call even if it is one of the many.

As withdraw() the only way for a user to exit pool, her funds will be frozen within.

If slope is set to zero for such a token, either maliciously or mistakenly, the withdrawals are impossible for all the users.

As this is user fund freeze case with external assumptions, setting the severity to medium.

Proof of Concept

Some ERC20 tokens do not allow for zero amount transfers:

https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers

withdraw() iterates across the set of reward tokens, and requires all transfers to go through:

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

success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);

Once some is not ok, the whole call reverts. As it's the only way for a user to exit the pool, her funds are frozen until non-zero reward is obtained.

It might never happen as rewardsWeiPerSecondPerToken is allowed to be zero:

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

    function addPool (
        uint startTime,
        uint maxDeposit,
        uint[] memory rewardsWeiPerSecondPerToken,
        uint programLengthDays,
        address depositTokenAddress,
        address excessBeneficiary,
        address[] memory rewardTokenAddresses,
        bytes32 ipfsHash,
        bytes32 name
    ) external {
        Pool storage pool = pools[++numPools];
        pool.id = numPools;
        pool.rewardsWeiPerSecondPerToken = rewardsWeiPerSecondPerToken;
        pool.startTime = startTime > block.timestamp ? startTime : block.timestamp;
        pool.endTime = pool.startTime + (programLengthDays * 1 days);
        pool.depositToken = depositTokenAddress;
        pool.excessBeneficiary = excessBeneficiary;
        pool.taxPerCapita = globalTaxPerCapita;

        require(rewardsWeiPerSecondPerToken.length == rewardTokenAddresses.length, 'Rewards and reward token arrays must be same length');

This way, when one of the reward tokens doesn't allow for zero transfers:

1. immediate withdraw after deposit is impossible
2. factory allows for creation of malicious or misconfigured pools by adding such a reward token with zero rewardsWeiPerSecondPerToken, making withdraw impossible for all users

Recommended Mitigation Steps

Consider controlling for zero amounts in reward transfer cycle:

        for (uint i = 0; i < rewards.length; i++) {
+	        if (rewards[i] > 0) {
	            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);
+	        }
        }

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

Lines of code

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

Vulnerability details

Once reward/deposit tokens decimals differ from 18 the calculations with a hard coded 1e18 will become grossly incorrect.

This will lead either to receiving no rewards: say deposit is USDC with decimals of 6, being divided by 1e18 it adds 1e-12 to the rewards calculations, making it close to zero.

When it is the opposite it's more severe as first reward takers will simply empty the pool.

Proof of Concept

Rewards calculation mixes up the reward and deposit token amounts as if both have decimals of 18:

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

   rewardsLocal[i] = (secondsDiff * pool.rewardsWeiPerSecondPerToken[i] * receipt.amountDepositedWei) / 1e18;

getMaximumRewards also uses hard coded 1e18:

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

return pool.rewardsWeiPerSecondPerToken[rewardIndex] * pool.maximumDepositWei * (pool.endTime - pool.startTime) / 1e18;

Recommended Mitigation Steps

Record and take into account reward and deposit token decimals.

1. Excess ether sent to FixedPricePassThruGate is lost (low)

passThruGate() redirects to a beneficiary only gate.ethCost, requiring that msg.value >= gate.ethCost. As there are no other ways to access native tokens held by this contract, the cumulative excess value, a sum of msg.value - gate.ethCost, will be permanently frozen within the contract.

Proof of Concept

Only gate.ethCost is now forwarded:

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

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

Recommended Mitigation Steps

Pass-though the whole msg.value as the excess is not used:

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

2. One step owner change (non-critical)

One step process offers no protection for the cases when ownership transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the change.

Proof of Concept

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L148-L155

    /// @notice Changing the owner key
    /// @dev Only current owner may do this
    /// @param newOwner the new address that will be owner, old address is no longer owner
    function setOwner(address newOwner) external ownerOnly {
        address oldOwner = _owner_;
        _owner_ = newOwner;
        emit OwnerUpdated(oldOwner, newOwner);
    }

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

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

Recommended Mitigation Steps

Consider utilizing two-step owner rights transferring process (proposition and acceptance in the separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system.

3. No zero address checks in constructors (non-critical)

Functionality of the system will be unavailable if core config addresses be set to zero.

Proof of Concept

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

    /// @notice Whoever deploys the contract sets the two privileged keys
    /// @param _mgmt key that will initially be both management and treeAdder
    constructor(address _mgmt) {
        management = _mgmt;
        treeAdder = _mgmt;
    }

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

    /// @notice Deployer connects it to MerkleIdentity
    /// @param _gateMaster address of MerkleIdentity contract, which has exclusive right to call passThruGate
    constructor(address _gateMaster) {
        gateMaster = _gateMaster;
    }

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/VoterID.sol#L102-L114

    /// @notice Whoever deploys the contract determines the name, symbol and owner. Minter should be MerkleIdentity contract
    /// @dev names are misspelled on purpose because we already have owners and _owner_ and _name and...
    /// @param ooner the owner of this contract
    /// @param minter address (MerkleIdentity contract) that can mint NFTs in this series
    /// @param nomen name of the NFT series
    /// @param symbowl symbol for the NFT series
    constructor(address ooner, address minter, string memory nomen, string memory symbowl) {
        _owner_ = ooner;
        // we set it here with no resetting allowed so we cannot commit to NFTs and then reset
        _minter = minter;
        _name = nomen;
        _symbol = symbowl;
    }

Recommended Mitigation Steps

Consider adding zero address checks as operation mistakes mitigation tends to out-weight gas optimization in such core config one time setups.