Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L80-L91 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L107-L123

Vulnerability details

Impact

This nature of this bug is similar in MerkleVesting and MerkleResistor and MerkleDropFactory, so I only write MerkleDropFactory version: If a user calls depositTokens() with wrong treeIndex value by mistake, attacker can perform front-running attack and steal user funds (before user transaction reach the contract, attacker will create multiple trees in MerkleDropFactory until he reach tree with index equal to the treeIndex, then he can set specially crafted parameters for that tree and force MerkleDropFactory to transfer user tokens(attacker can choose any token that user approved spending for MerkleDropFactory in those tokens) and then attacker can withdraw that tokens and steal them (because attacker created tree, so he specify the tree withdraw data) )

Proof of Concept

This is depositTokens() code:

    function depositTokens(uint treeIndex, uint value) public {
        // storage since we are editing
        MerkleTree storage merkleTree = merkleTrees[treeIndex];

        // bookkeeping to make sure trees don't share tokens
        merkleTree.tokenBalance += value;

        // transfer tokens, if this is a malicious token, then this whole tree is malicious
        // but it does not effect the other trees
        require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
        emit TokensDeposited(treeIndex, merkleTree.tokenAddress, value);
    }

As you can see, there is no check for treeIndex in the code but if user specify wrong treeIndex which is not initialized yet then contract will not transfer any tokens because merkleTree.tokenAddress will be 0x0. so user expect that if he enter wrong uninitialized treeIndex then nothing bad could happen. But attacker can interfere and make user to deposit his tokens to malicious tree created by attacker, then attacker will withdraw user tokens. To perform this attacker will do this:

Attacker will create a smart contract that calls addMerkleTree of MerkleDropFactory for specified times and specified parameters.
attacker create a bot and will watch new transactions in mem pools to find a user's transaction that calls depositTokens() with wrong treeIndex and treeIndex is not so much higher than numTrees. (let's assume user wrong treeIndex is TREEINDEX)
attacker's bot will create a transaction with higher gas price which calls attacker's contract and the contract will create (numTrees-TREEINDEX) trees in MerkleDropFactory with attacker specified parameters. (the new tree will set tokenAddress as one of tokens that user approved spending of it for MerkleDropFactory) ((numTrees-TREEINDEX) value shouldn't be so high and the gas amount should be lower than user deposit value that attacker is going to steal, in most mistakes it's going to be 1 or 2)
after attacker contract created malicious trees and specified parameters for the TREEINDEX (the index in user transaction) with front-running attack, then user transaction will be mined and contract will accept that transaction and it will transfer user tokens to contract address(attacker can choose which token).
because attacker specified parameters for tree TREEINDEX, so he can control merkleRoot and withdraw information and can withdraw user funds after user depositTokens() transaction.

The root cause of this issue is that depositTokens() only accepts treeIndex to specify the tree. but treeIndex has no identity information, anyone can be creator of that treeIndex between user signing that transaction that mining of that transaction. so if user put wrong uninitialized treeIndex attacker can steal any of his tokens that he approved spending for MerkleDropFactory. And also if user creates a tree, he can't know that what is his tree's treeIndex and attacker can create copy's of all trees before user tree and after it (sandwich attack), and make users to deposit their tokens to his trees by mistake(if user use some logic that uses numTrees value to find last tree before or after his transaction, attacker can create tree before and after user transaction and exploit that logic). The only easy way that user can be assure that he deposited tokens to his own tree(after tree creation) is that he do them in same transaction with smart contract coding(he shouldn't call any other contract between creation and depositing), in any other way the chance of fund loss is very high.

Tools Used

VIM

Recommended Mitigation Steps

Depositing by specifying only treeIndex is not secure, users should send merkleRoot hash to deposit too, and contract can check that if that treeIndex and merkleRoot is not a match then revert. and also if merkleRoot of that tokenIndex is 0x0 then revert. in this way attacker can never interrupt user's transactions with wrong values.

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