code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

If user call withdraw() of addMerkleTree with uncivilized merkleIndex, then it's possible for treeAdder to perform front-running attack and steal his funds.

Proof of Concept

This is code of withdraw() in addMerkleTree:

    function withdraw(uint merkleIndex, uint tokenId, string memory uri, bytes32[] memory addressProof, bytes32[] memory metadataProof) external payable {
        MerkleTree storage tree = merkleTrees[merkleIndex];
        IVoterID id = IVoterID(tree.nftAddress);

        // mint an identity first, this keeps the token-collision gas cost down
        id.createIdentityFor(msg.sender, tokenId, uri);

        // check that the merkle index is real
        require(merkleIndex <= numTrees, 'merkleIndex out of range');

        // verify that the metadata is real
        require(verifyMetadata(tree.metadataMerkleRoot, tokenId, uri, metadataProof), "The metadata proof could not be verified");

        // check eligibility of address
        IEligibility(tree.eligibilityAddress).passThruGate(tree.eligibilityIndex, msg.sender, addressProof);

        // check that the price is right
        IPriceGate(tree.priceGateAddress).passThruGate{value: msg.value}(tree.priceIndex, msg.sender);
    }

In normal scenario this code will revert if merkleIndex was uncivilized because of this line: require(merkleIndex <= numTrees, 'merkleIndex out of range');. so user except his transaction with wrong merkleIndex will be reverted, but a malicious tokenAdder can detect these transactions in mem-pools and perform front-running attack and create enough merkle tree in contract that he reaches user specified index and set that tree parameters in a way that he can steal user funds and mint no NFT for user(he can implement this logic in pre deployed contract, and just call that contract in front-running).

Tools Used

VIM

Recommended Mitigation Steps

User should be able to specify a hash of all the information of tree, so if user specify wrong values by mistake, attacker couldn't be able to steal user funds.

illuzen commented 2 years ago

Out of scope, frontend attack, also duplicate, also if treeAdder is malicious, entire contract could be maliciously configured

gititGoro commented 2 years ago

Owner's comments stand. Making invalid.

code-423n4 / 2022-05-factorydao-findings