code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

If treeAdder call addMerkleTree() of MerkleIdentity with wrong values for eligibilityIndex or priceIndex (uninitialized gates index) attacker can create those gate indexes in priceGateAddress or eligibilityAddress (they are permission less) with his own specific parameters and steal NFTs just by minting a lot for himself or by buying for low prices.

Proof of Concept

This is the code of addMerkleTree() in MerkleIdentity:

    function addMerkleTree(
        bytes32 metadataMerkleRoot,
        bytes32 ipfsHash,
        address nftAddress,
        address priceGateAddress,
        address eligibilityAddress,
        uint eligibilityIndex,
        uint priceIndex) external {
        require(msg.sender == treeAdder, 'Only treeAdder can add trees');
        MerkleTree storage tree = merkleTrees[++numTrees];
        tree.metadataMerkleRoot = metadataMerkleRoot;
        tree.ipfsHash = ipfsHash;
        tree.nftAddress = nftAddress;
        tree.priceGateAddress = priceGateAddress;
        tree.eligibilityAddress = eligibilityAddress;
        tree.eligibilityIndex = eligibilityIndex;
        tree.priceIndex = priceIndex;
        emit MerkleTreeAdded(numTrees, nftAddress);
    }

As you can see there is no check that (eligibilityIndex in eligibilityAddress) or (priceIndex in priceGateAddress) are valid gate index and initialized in the past and even there were some checks attacker could still do this by front-running, attacker only need treeAdder to set wrong values for eligibilityIndex or priceIndex (uninitialized gates index), then attacker can create gates for those indexes by front-running or normal transactions. (gates can be created by anyone, and attacker create bunch of them in his own contract in one transaction to reach those indexes).

If treeAdder by mistake set wrong values for them, then attacker can create those gates with specially crafted parameters in eligibilityAddress or priceGateAddress (create enough gates that numGates reaches eligibilityIndex or priceIndex) and manipulate price or minting permission of the NFT token and steal funds. becasuse for price or minting permission MerkleIdentity() calls those gates:

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

Tools Used

VIM

Recommended Mitigation Steps

specifying eligibilityIndex or priceIndex to identify gates are not enough, there should be some other variable like creator to create link for gates with (index, creator), so this kind of attack in case of wrong values by mistake don't be possible by attacker.

illuzen commented 2 years ago

Incorrect argument entry is out of scope, this is a frontend attack. Almost all contracts have some configuration that if done improperly could be bad.

IllIllI000 commented 2 years ago

The sponsor has marked multiple findings as invalid, stating that incorrect arguments aren't in scope https://github.com/code-423n4/2022-05-factorydao-findings/search?q=%22not+in+scope%22++label%3Ainvalid+%22arguments%22&type=issues. However, nowhere in the readme does it mention this. Some wardens spent a lot of time finding and filing issues for things they wouldn't have had this been mentioned in the readme. In some of the issues, funds are at risk. Also, the sponsor mentions things as being invalid due to being a part of the front-end. If someone interacts directly with the contracts, they won't be using the front-end.

gititGoro commented 2 years ago

The sponsor didn't mark anything invalid. The judge did. The sponsor made the case and the judge either agrees or disagreed.

For parameter induced errors, what a warden should look for is when a error occurs for values that are expected to produce no error.

For instance, take the fictional function

/**
* @dev setting time to zero just returns displacement
*/ 
function calculateVelocity(uint displacement, uint time) external pure returns (uint) {
return displacement/time;
}

Here, the dev has incorrectly assumed that time=0 will pass when in fact it will revert. A warden should report this.

But now consider the following contract

contract CarStats{
   uint8 massOfCarInKg = 1000; //assume 1 ton as default
   constructor(uint8 _mass){ 
       mass = mass;
   }
   function calculateMomentum (uint velocity) external pure returns(uint) {
     return velocity*uint(mass);
   }
}

If the warden reports that inputting a very high velocity can lead to overflow then they're implying that the user wants a car that can travel many times faster than the speed of light. In this case the report is invalid.

The sponsor is correct that "Almost all contracts have some configuration that if done improperly could be bad." and doesn't have to provide explanation of a universal truth in a README.

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