Critical ItchyDAO Bug in the Voting Contract of MakerDAO

Around 2019/05/07 00:30 a.m. (UTC+8), blockchain company Zeppelin announced a security warning on the most famous DeFi project — MakerDAO, declared that there is a critical vulnerability in the voting contract which might affect users who have staked MKR, and advised them to move their MKR out of the old contract and back into personal wallet immediately.

According to their medium post, the vulnerability has now been fixed.

The new contract is not open-sourced yet, however, after PeckShieldthoroughly reviewed the original voting contract, we believe we’ve identified the bug. And it’s confirmed by the Maker foundation after we discussed with their engineer. As claimed by Zeppelin, the vulnerability (we named it Itchy DAO) is critical and could be used to compromise the voting result, and more worse, could possibly cause some MKR tokens staked in the contract unable to retrieve back and stay locked forever.

Here we will disclose the details, and hope every affected projects could fix it ASAP.

Details

In the MakerDAO system, users could lock up their MKR to vote and participate the governance, you can refer to FAQ for more details.

Let us take a closer look in the smart contract and see how it works. You can lock / free your MKR tokens for voting or canceling your vote

function lock(uint wad)
    public
    note
{
    GOV.pull(msg.sender, wad);
    IOU.mint(msg.sender, wad);
    deposits[msg.sender] = add(deposits[msg.sender], wad);
    addWeight(wad, votes[msg.sender]);
}
function free(uint wad)
    public
    note
{
    deposits[msg.sender] = sub(deposits[msg.sender], wad);
    subWeight(wad, votes[msg.sender]);
    IOU.burn(msg.sender, wad);
    GOV.push(msg.sender, wad);
}

After locking your MKR, you can vote for several proposals (which are represented by an address array) that interest you.

mapping(bytes32=>address[]) public slates;
mapping(address=>bytes32) public votes;
mapping(address=>uint256) public deposits;
function etch(address[] yays)
    public
    note
    returns (bytes32 slate)
{
    require( yays.length <= MAX_YAYS );
    requireByteOrderedSet(yays);
bytes32 hash = keccak256(yays);
    slates[hash] = yays;
    Etch(hash);
    return hash;
}
function vote(address[] yays) public returns (bytes32)
    // note  both sub-calls note
{
    var slate = etch(yays);
    vote(slate);
    return slate;
}
function vote(bytes32 slate)
    public
    note
{
    uint weight = deposits[msg.sender];
    subWeight(weight, votes[msg.sender]);
    votes[msg.sender] = slate;
    addWeight(weight, votes[msg.sender]);
}

There are two kinds of vote() accept two different kinds of parameters (address array and byte32).

Actually vote(address[] yays) will call vote(bytes32 slate) in the end, you can refer to figure 1 for the flow.

Figure 1: The flow of vote(address[] yays)

In a simple word, these 2 vote() will call addWeight() to vote for the proposals you like.

mapping(bytes32=>address[]) public slates;
mapping(address=>uint256) public approvals;
function addWeight(uint weight, bytes32 slate)
    internal
{
    var yays = slates[slate];
    for( uint i = 0; i < yays.length; i++) {
        approvals[yays[i]] = add(approvals[yays[i]], weight);
    }
}
function subWeight(uint weight, bytes32 slate)
    internal
{
    var yays = slates[slate];
    for( uint i = 0; i < yays.length; i++) {
        approvals[yays[i]] = sub(approvals[yays[i]], weight);
    }
}

Unfortunately, there is a misuse of function scope that could be exploited by a hacker to manipulate the vote result, even make some MKR tokens be locked forever.

Here is the scenario:

Assume a hacker who never votes nor locks any MKR before is now planning to attack the voting contract.

1. Call lock() to lock MKR, deposits[msg.sender] will store corresponding quantity for voting.

2. Calculate the sha3 hash (slate) for the target proposals that will be used in step 3.

The key here is to pick up target proposals that the calculated hash for the combination doesn’t exist in slates[] yet. You might need to intentionally include a brand new proposal as a salt to achieve this goal. The calculation can be done offline since it just simply calls sha3 hash function.

Figure 2: Precalculate the sha3 hash (slate)

3. Call vote(bytes32 slate) with the slate precalculated from step

Because votes[msg.sender] hasn’t assigned any value, subWeight() will return immediately (yays.length = 0), then votes[msg.sender] will store the hash value (slate) we provided. addWeight() will be called in the end, however, it will also return due to the same reason as the above subWeight().

4. Call etch() with target proposals (address[])

The array hash will finally be stored into slates[]. Be informed that etch() and vote() are all public and thus can be called by any user.

Figure 3: Call etch() to assign sha3 hash (slate)

5. Call free() to unlock MKR, it could be divided to the following steps: * deposits[msg.sender] = sub(deposits[msg.sender], wad), unlock the MKR tokens locked in step 1. * subWeight(wad, votes[msg.sender]), subtract votes from the proposals which our attacker never votes for them!

Through the above analysis, we can conclude that the attack has the following effects: * Reduce the voting counts of the victim proposals. * Lock certain MKR tokens forever in the contract.

Timeline

About Us

PeckShield Inc. is a leading blockchain security company with the goal of elevating the security, privacy, and usability of current blockchain ecosystem. For any business or media inquiries (including the need for smart contract auditing), please contact us at telegram, twitter, or email.