FactoryDAO contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

If user pay extra ether for minting NFT, then those extra ethers will be locked in FixedPricePassThruGate forever. because passThruGate() of FixedPricePassThruGate transfer only NFT cost to gate.beneficiary and don't return extra amount in msg.value to buyer or gate.beneficiary and that extra amount will be locked in contract forever because there is no functionality in FixedPricePassThruGate to withdraw them.

Proof of Concept

This is passThruGate() of FixedPricePassThruGate code:

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

In the begging of the function it checks that require(msg.value >= gate.ethCost, 'Please send more ETH'), so the function can be called with extra msg.value and It transfer gate.ethCost ether to gate.beneficiary and don't send remaining 'msg.value' to buyer or anywhere and those ethers will be locked in contract forever.

Also in withdraw() of MerkleIdentity we have this line:

        IPriceGate(tree.priceGateAddress).passThruGate{value: msg.value}(tree.priceIndex, msg.sender);

which calls priceGate with all the amount user payed, so if user pay extra ether then that extra amount will be lost forever.

Tools Used

VIM

Recommended Mitigation Steps

change passThruGate to be like this:

(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/PermissionlessBasicPoolFactory.sol#L156-L173

Vulnerability details

Impact

In getRewards() of PermissionlessBasicPoolFactory contract, there is a check to see that receipt is initialized receipt, but the condition used by code will be true for receiptId equal 0. because receiptId==0 is not initilized for any pool and the value of pools[poolId].receipts[0].id will be 0 so the condition receipt.id == receiptId will be passed on getRewards(). Any function that depends on getRewards() to check that if receptId has deposited fund, can be fooled. right now this bug has no direct money loss, but this function doesn't work as it suppose too.

Proof of Concept

This is getRewards() code:

    function getRewards(uint poolId, uint receiptId) public view returns (uint[] memory) {
        Pool storage pool = pools[poolId];
        Receipt memory receipt = pool.receipts[receiptId];
        require(pool.id == poolId, 'Uninitialized pool');
        require(receipt.id == receiptId, 'Uninitialized receipt');
        uint nowish = block.timestamp;
        if (nowish > pool.endTime) {
            nowish = pool.endTime;
        }

        uint secondsDiff = nowish - receipt.timeDeposited;
        uint[] memory rewardsLocal = new uint[](pool.rewardsWeiPerSecondPerToken.length);
        for (uint i = 0; i < pool.rewardsWeiPerSecondPerToken.length; i++) {
            rewardsLocal[i] = (secondsDiff * pool.rewardsWeiPerSecondPerToken[i] * receipt.amountDepositedWei) / 1e18;
        }
        return rewardsLocal;
    }

if the value of receiptId set as 0 then even so receiptId==0 is not initialized but this line:

        require(receipt.id == receiptId, 'Uninitialized receipt');

will be passed, because, receipts start from number 1 and pool.receipts[0] will have zero value for his fields. This is the code in deposit() which is responsible for creating receipt objects.

        pool.totalDepositsWei += amount;
        pool.numReceipts++;

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

as you can see pool.numReceipts++ and pool.receipts[pool.numReceipts] increase numReceipts and use it as receipts index. so receipnts will start from index 1. This bug will cause that getRewards(poolId, 0) return 0 instead of reverting. any function that depend on reverting of getRewards() for uninitialized receipts can be excploited by sending receipntId as 0. this function can be inside this contract or other contracts. (withdraw use getRewards and we will see that we can create WithdrawalOccurred event for receiptsId as 0)

Tools Used

VIM

Recommended Mitigation Steps

If you want to start from index 1 then add this line too to ensure receipntId is not 0 too:

require(receiptId > 0, 'Uninitialized receipt');

or we could check for uninitialized receipnts with owner field as non-zero.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L88-L109

Vulnerability details

Impact

In MerkleDropFactory there is a variable withdrawn[recipient][treeIndex] = hasUserWithdrawnAirdrop which specify that if some recipient received his tokens in treeIndex. but because code don't store leaf data or amount data in this variable, so if there were two leaf with same destination address, then user can only withdraw one of them and the remaining user fund(if deposited in contract) will be locked in MerkleDropFactory forever. and because withdraw() can be called by anyone, attacker can call withdraw() with leaf that has small amount and make user to get low amount of tokens (if one of the user's leafs in the tree are created by mistake and has 0 amount)

Proof of Concept

This is withdrawn() code:

    function withdraw(uint treeIndex, address destination, uint value, bytes32[] memory proof) public {
        // no withdrawing from uninitialized merkle trees
        require(treeIndex <= numTrees, "Provided merkle index doesn't exist");
        // no withdrawing same airdrop twice
        require(!withdrawn[destination][treeIndex], "You have already withdrawn your entitled token.");
        // compute merkle leaf, this is first element of proof
        bytes32 leaf = keccak256(abi.encode(destination, value));
        // storage because we edit
        MerkleTree storage tree = merkleTrees[treeIndex];
        // this calls to MerkleLib, will return false if recursive hashes do not end in merkle root
        require(tree.merkleRoot.verifyProof(leaf, proof), "The proof could not be verified.");
        // close re-entrance gate, prevent double claims
        withdrawn[destination][treeIndex] = true;
        // update struct
        tree.tokenBalance -= value;
        tree.spentTokens += value;
        // transfer the tokens
        // NOTE: if the token contract is malicious this call could re-enter this function
        // which will fail because withdrawn will be set to true
        require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed");
        emit WithdrawalOccurred(treeIndex, destination, value);
    }

As you can see in withdraw contract set withdrawn[destination][treeIndex] = true and also in the beginning it checks that: require(!withdrawn[destination][treeIndex], "You have already withdrawn your entitled token."). So if there were two leaf with same destination value (for example one of them is created by mistake with amount=0) then attacker can do this steps:

1. call withdraw() with one of the destination's leafs info, that has lower amount (could be zero if that leaf added there by mistake).
2. contract will make withdrawn[destination][treeIndex] = true for that destination.
3. user: destination can't call withdraw() to get his other leaf (real one) tokens because contract will check and see that this user has already withdrawn his funds.
4. If creator deposited that tokens in MerkleDropFactory then they will be locked forever.

Tools Used

VIM

Recommended Mitigation Steps

contract should keep track of every leaf withdraws, and don't assume that for each destination we only have one leaf.