ctf_sec - Funds transferred to an Vote Delegate Pool can be drained by an attacker via a hash collision.

[sherlock-admin4]

ctf_sec

Medium

Funds transferred to an Vote Delegate Pool can be drained by an attacker via a hash collision.

Lines of code

https://github.com/makerdao/vote-delegate/blob/ae29376d2b8fdb7293c588584f62fe302914f575/src/VoteDelegateFactory.sol#L61C14-L61C22

https://github.com/makerdao/vote-delegate/blob/ae29376d2b8fdb7293c588584f62fe302914f575/src/VoteDelegate.sol#L65

Vulnerability details

Funds transferred to an Vote Delegate Pool can be drained by an attacker via a hash collision.

Proof of Concept (NOTE: This report is inspired from this past valid report. Necessary changes have been made to suit the maker dao Protocol.)

The attack consists of two parts: Finding a collision and actually draining the lending pool. We describe both here:

PoC: Finding a collision

https://github.com/makerdao/vote-delegate/blob/ae29376d2b8fdb7293c588584f62fe302914f575/src/VoteDelegateFactory.sol#L61C14-L61C22

The function create() from VoteDelegateFactory compute the salt solely from msg.sender address, which means, the final address where the contract will be deployed relies solely on the parameters of the constructor of the newly created contract, in this case, the address chief, address polling, address delegate_

the address chief and address pool is fixe and does not needs to be brute forced.

the attack only needs to predict the address delegate.

the address delegate can be some famous governance address (maker dao governance address, etc...)

https://github.com/makerdao/vote-delegate/blob/ae29376d2b8fdb7293c588584f62fe302914f575/src/VoteDelegate.sol#L65

function create() external returns (address voteDelegate) { voteDelegate = address(new VoteDelegate{salt: bytes32(uint256(uint160(msg.sender)))}(chief, polling, msg.sender)); created[voteDelegate] = 1;

    emit CreateVoteDelegate(msg.sender, voteDelegate);
}

The address collision an attacker will need to find are:

The address where an delegate address would be deployed to (1).

Arbitrary attacker-controlled wallet contract (2).

Both sets of addresses can be brute-force searched because:

As shown above, salt is set derived from msg.sender, thus, the final address is determined only by the three parmeters passed to the constructor. By brute-forcing many address values, we have obtained many different (undeployed) address accounts for (1). The user can know the address of vote delegate pool before deploying it,

(2) can be searched the same way. The contract just has to be deployed using CREATE2, and the salt is in the attacker's control by definition. An attacker can find any single address collision between (1) and (2) with high probability of success using the following meet-in-the-middle technique, a classic brute-force-based attack in cryptography:

Brute-force a sufficient number of values of delegate address (2^80), pre-compute the resulting account addresses, and efficiently store them e.g. in a Bloom filter data structure.

Brute-force contract pre-computation to find a collision with any address within the stored set in step 1. The feasibility, as well as detailed technique and hardware requirements of finding a collision, are sufficiently described in multiple references:

1: A past issue on Sherlock describing this attack. 2: EIP-3607, which rationale is this exact attack. The EIP is in final state. 3: A blog post discussing the cost (money and time) of this exact attack. The hashrate of the BTC network has reached 6.5x10^20 hashes per second as of time of writing, taking only just 31 minutes to achieve 2^80 hashes. A fraction of this computing power will still easily find a collision in a reasonably short timeline.

PoC: Draining the lending pool Even given EIP-3607 which disables an EOA if a contract is already deployed on top, we show that it's still possible to drain the Vote Delegate Pool entirely given a contract collision.

Assuming the attacker has already found an address collision against an undeployed vote delegate pool, let's say 0xCOLLIDED. The steps for complete draining of the Vote Delegate Pool are as follow:

First tx:

Deploy the attack contract onto address 0xCOLLIDED. Set infinite allowance for {0xCOLLIDED ---> attacker wallet} for any token they want. Destroy the contract using selfdestruct. Post Dencun hardfork, selfdestruct is still possible if the contract was created in the same transaction. The only catch is that all 3 of these steps must be done in one tx.

The attacker now has complete control of any funds sent to 0xCOLLIDED.

Second tx:

Deploy the address to 0xCOLLIDED. Wait until the contract will hold as many tokens as you want and drain it. The attacker has stolen all funds from the the contract.

Proof of Concept

While we cannot provide an actual hash collision due to infrastructural constraints, we are able to provide a coded PoC to prove the following two properties of the EVM that would enable this attack:

A contract can be deployed on top of an address that already had a contract before. By deploying a contract and self-destruct in the same tx, we are able to set allowance for an address that has no bytecode. Here is the PoC, as well as detailed steps to recreate it:

Paste the following file onto Remix (or a developing environment of choice):

POC

pragma solidity ^0.8.20;

contract Token {
    mapping(address => mapping(address => uint256)) public allowance;

    function increaseAllowance(address to, uint256 amount) public {
       allowance[msg.sender][to] += amount;
    }
}

contract InstantApprove {
    function setApprove(Token ts, uint256 amount) public {
        ts.increaseAllowance(msg.sender, amount);
    }

    function destroy() public {
        selfdestruct(payable(tx.origin));
    }
}

contract Test {
    Token public ts;
    uint256 public constant APPROVE_AMOUNT = 2e18;

    constructor() {
        ts = new Token();
    }

    function test(uint _salt) public returns (address) {
        InstantApprove ia = new InstantApprove{salt: keccak256(abi.encodePacked(_salt))}();
        address ia_addr = address(ia);

        ia.setApprove(ts, APPROVE_AMOUNT);
        ia.destroy();
        return ia_addr;
    }

    function getCodeSize(address addr) public view returns (uint) {
        uint size;
        assembly {
            size := extcodesize(addr)
        }
        return size;
    }

    function getAllowance(address from) public view returns (uint) {
        return ts.allowance(from, address(this));
    } 
}

Deploy the contract Test.

Run the function Test.test() with a salt of your choice, and record the returned address. The result will be:

Test.getAllowance() for that address will return exactly APPROVE_AMOUNT. Test.getCodeSize() for that address will return exactly zero.

This proves the second property.

Using the same salt in step 3, run Test.test() again. The tx will go through, and the result will be:

Test.test() returns the same address as with the first run. Test.getAllowance() for that address will return twice of APPROVE_AMOUNT. Test.getCodeSize() for that address will still return zero. This proves the first property.

Tools Used

Manual Audit

Recommended Mitigation Steps

Don't use an preventable salt to create the pools.

Duplicate of #63