fund loss for user and owner of tree if there were two leaf for same user in MerkleVesting's or MerkleResistor's MerkleTree

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleResistor.sol#L134-L162 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L104-L132

Vulnerability details

Impact

In function initialize() of MerkleVesting or MerkleResistor there is a variable initialized[recipient][treeIndex] = wasItInitialized which specify that if some destination initialized his process in treeIndex and tranches[destination][treeIndex] has been set, so withdraw() can be called after initialize(). but because code don't store leaf data or amount data in this tranches[destination][treeIndex] or initialized[recipient][treeIndex], so if there were two leaf with same destination address, then than destination can only withdraw one of them and the remaining user fund(if deposited in contract) will be locked in MerkleVesting or MerkleResistor forever. and because withdraw() can be called by anyone (in MerkleVesting), attacker can call withdraw() with leaf info 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 totalCoins user will receive 0 token)

Proof of Concept

This is initialize() code in MerkleVesting:

    function initialize(uint treeIndex, address destination, uint totalCoins, uint startTime, uint endTime, uint lockPeriodEndTime, bytes32[] memory proof) external {
        // must not initialize multiple times
        require(!initialized[destination][treeIndex], "Already initialized");
        // leaf hash is digest of vesting schedule parameters and destination
        // NOTE: use abi.encode, not abi.encodePacked to avoid possible (but unlikely) collision
        bytes32 leaf = keccak256(abi.encode(destination, totalCoins, startTime, endTime, lockPeriodEndTime));
        // memory because we read only
        MerkleTree memory tree = merkleTrees[treeIndex];
        // call to MerkleLib to check if the submitted data is correct
        require(tree.rootHash.verifyProof(leaf, proof), "The proof could not be verified.");
        // set initialized, preventing double initialization
        initialized[destination][treeIndex] = true;
        // precompute how many coins are released per second
        uint coinsPerSecond = totalCoins / (endTime - startTime);
        // create the tranche struct and assign it
        tranches[destination][treeIndex] = Tranche(
            totalCoins,  // total coins to be released
            totalCoins,  // currentCoins starts as totalCoins
            startTime,
            endTime,
            coinsPerSecond,
            startTime,    // lastWithdrawal starts as startTime
            lockPeriodEndTime
        );
        // if we've passed the lock time go ahead and perform a withdrawal now
        if (lockPeriodEndTime < block.timestamp) {
            withdraw(treeIndex, destination);
        }
    }

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

1. call withdraw() with one of the destination's leafs info, that has lower totalCoins (could be zero if that leaf added there by mistake).
2. contract will make initialized[destination][treeIndex] = true for that destination.
3. user: destination can't call initialized() to get his other leaf (real one) tokens because contract will check and see that this user has already initialized his withdraw process.
4. If creator deposited that destination's tokens in MerkleVesting 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.

[illuzen]

duplicate #148

[gititGoro]

Reducing severity