ensdomains / ens Goto Github PK

My analysis of what happens when the value of the bid is higher than the
amount submitted. Thanks @rfikki for inquiring about the behaviour related to underpaying for a bid.

1. When you submit a new bid, a deed contract is created. The full amount of ether you submitted (let's say 3 ETH) is sent to that Deed.

2. When you then unseal your bid, revealing the value of your actual bid (let's say 3 ETH) one of two things can happen:

   1. The current highest bid for foobarbaz.eth is 4 ETH. You do not win. The registrar thus closes the deed, refunding 99.9% of the amount submitted.

   2. The current highest bid for foobarbaz.eth is 2 ETH. You are the highest bidder so far! In this case, the registrar will try to update the balance held by the deed, and refund the difference between your balance (3 ETH) and your bid (6 ETH). But it expects your balance to be greater than your bid value, when it turns out that's not true, it throws.

From what I can see it looks like that's a lopsided outcome.

If you bid more than you submit and you are the highest bidder, then your funds are locked in the deed. (Using Deed.setBalance)
If you bid more than you submit and you are NOT the highest bidder, then your funds will be refunded. (using Deed.closeDeed)

The key difference is that Deed.setBalance actually compares your balance to your bid value. Deed.closeDeed doesn't.

From Alex's list -

• Check if latest contract changes affect app functionality
• Display “download mist/metamask” message when on non-blockchain browser
• Approve and update texts and legal disclaimers
• Requires: contract deployed
• Add final addresses to js libraries and update them
• Deploy code to github, and swarm

Chris will post what @Arachnid drafts from #107

Could be on Medium.com, or other popular forum. I am working on one, but the more the better.

The cancelBid function has no modifiers and it does not check msg.sender. It takes two arguments that can be guessed from public information. And it does invalidate a bid.

So it seems like anybody can cancel bids. Is this a problem?

https://github.com/ethereum/ens/blob/c8ea65a08a40e0aa9b701ba5bd5f624ea413e8de/HashRegistrarSimplified.sol#L371

What does this means?

Attention: You must reveal your bid within 24 hours of the reveal phase, failing to do so will result in irreversible loss of funds

And this?

The top bidder will own the name, but you will be refunded of any amount more than the strictly necessary to overbid the second place, therefore you only need to bid once. Your maximum bid is kept secret until you reveal it.

Why i need to reveal my bid? What is protect anonymity? What is this %? If some name already bidded like "insurance" why cant i post my bid to this?

What does those titles means

Points to: 0x18ee4d627b7c1951a209b75a20e3296e434a4ad6

Owned by: 0x8394a052eb6c32fb9defcaabc12fcbd8fea0b8a8

Deed address: 0x268e06911ba1ddc9138b355f9b42711abbc6eaec

Content: not set

Pending auditor feedback.

Ideally, via a 'deployment contract' that takes care of creating all the necessary contracts, then selfdestructs (or sits there and returns the relevant addresses for you)

• Finalize agreement and sent to them
• Get all holders to agree to it
• Publicize the keyholders and their function

...by throwing in their receive function

See Deployment and Upgrade process of ethereum/EIPs#162

From Alex's list -

• approve texts

Alex wants to add additional restrictions to the registrar for launch: either a maximum auction value (above which any bids are refunded) or a longer minimum name length, in order to have a softer launch with less funds invested.

Nick is concerned that doing that is likely to cripple the name system's usefulness, or introduce extra game-theoretic issues we haven't thought through.

It seems that Deed's value is always supposed to be less or equal to Deed's balance. Though, Deed creation, as it is implemented in Registrar smart contract, is not atomic. Deed is first created with some value and zero balance (https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L305) and then its balance is resupplied with exact same value (https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L309).

Deed creation could be made atomic via payable constructor:

function Deed() payable {
    ...
    value = msg.value;
}

That could be called like this:

Deed newBid = (new Deed).value(msg.value)();

Such refactoring will make default function in Deed contract probably not needed.

@alexvandesande's design is well under way: https://projects.invisionapp.com/share/FE93G2K3Y#/screens/200024083

Registrar smart contract refunds only 0.5% of bid value to bidders who didn't unseal their bids in time. This is quite severe punishment, though it seems that bidder, who didn't reveal his bid in time may still get 99.5% or even 100% refund, he just needs to be patient enough. Here is what he needs to do:

1. Wait until the name, he bid for, will be released
2. Start another auction for the same name
3. Reveal the bid during reveal phase

If he will win this time, he will release the name later and get 100% refund, If he will loose, he will get 99.5% refund immediately.

Issue to track Martin's audit progress.

Suppose as an early adopter I manage to get google.eth for cheap. Supposing I don't want to give it up, what recourse would a company such as Google have to take the name? Its not clear to be based on the wiki here.

Since most addresses execute these two transactions one after the other, the loss in privacy is minimal, and this would allow UIs to be cleaner, since the user only has to approve one operation.

Suppose I were to buy justinmchase.eth, but then I want example.justinmchase.eth as well?

It seems like it would be nice to be able to have unlimited subs for any domain you may have acquired. Is this possible or planned? Or would I need to manage them all individually through auction? Or am I thinking about it all wrong?

I run npm test and the assert here https://github.com/ethereum/ens/blob/f24df55d4c7378e46be7b49085d6e9041be310f7/test/ens_test.js#L73 fails with, in short:

AssertionError: Error: VM Exception while executing transaction: invalid opcode
    at Object.callback (/home/vagrant/DAPPS/ens/test/ens_test.js:73:11)

My versions are:

• OS: vagrant-ubuntu-trusty-64
• NodeJs: v6.10.0
• Mocha: 3.2.0
• project's node_modules testrpc: 2.2.4
• project's node_modules solc: 0.4.10
• project's node_modules web3: 0.16.0

The test at line 53 passes.

I'm sure I'm just missing something dumb. So apologies in advance!

TL; DR: Should the comparisons in HashRegistrarSimplified.sol#L341 and HashRegistrarSimplified.sol#337 and HashRegistrarSimplified.sol#355 be against actualValue rather than _value?

Via shaBid(bytes32 hash, address owner, uint value, bytes32 salt), I can create a bid for associated with any value that I please. This value is part of the hash. I will always use this value to identify the bid. No ether is sent.

Via newBid(bytes32 sealedBid), I enter my bid into the auction. I may pay ether here, but there is no enforced relationship between the amount of ether I pay and the value I have declared when defining the bid. The value of my Deed will be defined by the amount I have paid, and the deed will own that amount.But this may not be the value I declared when defining the bid.

When I call unsealBid, I must identify the bid with its declared value _value, which may differ from the value of the Deed that represents it. The function will compute a quantity actualValue, which will be the minimum of the value I actually paid and the value declared. So, if I have underpaid, actualValue will be the value that I have actually paid.

However, when checking to see if I have won the auction (or if I have bid at least the minimum price), the declared value _value is used rather than actualValue to decide if I have won. If my declared value is sufficiently high, I eject the previous winner of the auction. The winning bid gets set to actualValue, which may be lower than my declared value, and lower than the prior winning bid.

So it looks to me like I could win auctions by declaring very high values but underfunding them (and unsealing my bids late in the reveal period, since my low actually-paid value becomes an easily displaceable highestBid).

I really do apologize for wasting your time if there is a constraint that I am not seeing that prevents this! I've tested nothing, I'm just trying to understand the code.

p.s. Thank you very much Martin Koeppelmann and Stefan George for taking a look at this! Any mistakes are obviously all my own, but they provided an extremely helpful quick review.

From Alex's list -

• Test and merge final vulnerability code changes
• Write up a text explaining how the deployment process works
• Deploy registrar contract
• Point “.eth” to the registrar contract

If we rely on the owner field of ENS for a domain, we can transparently support an upgrade process where old domains remain under the control of the original registrar - eg, we'll never try to auction off a domain that was given out by a previous registrar. This would make certain types of upgrade much simpler.

Not sure whether this is a bug or expected behaviour.

Function releaseDeed of Registrar smart contract reset owner of the node but does not reset resolver associated with it. It also does not do anything with descendants of the node. This means that node itself as well as its descendants will be resolvable even after deed was release. More over, former owner of the node will still be the owner of all descendant nodes.

If this is a bug, it should be fixed. If not, this behaviour should be clearly documented in EIP-162 or somewhere else.

If it will be owned by a multisig contract, also select/create the design of that contract.

EIP-162 says that for unsealing valid non-winning bid, the bidder gets 99.9% of bid value, while current implementation returns only 99.5%.

I deployed the following version to test the contract:

    function getRootOwner() constant returns(address) {
        return ens.owner(rootNode);
    }
    
    function getOwner(bytes32 label) only_owner(label) constant returns(address) {
        var node = sha3(rootNode, label);
        return ens.owner(node);
    }
    
    function registerDryRun(bytes32 subnode) only_owner(subnode) {
        Register(rootNode, subnode, msg.sender);
    }

    
    function register(bytes32 subnode) only_owner(subnode) {
        ens.setSubnodeOwner(rootNode, subnode, msg.sender);
    }

Error

Run out of the max limit gas on estimateGas.

Test Case

• rootNode(namehash('eth')) = 0x93cdeb708b7545dc668eb9280176169d1c33cfd8ed6f04690a0bcc88a93fc4ae
• subnode(sha3('foo')) = 0x41b1a0649752af1b28b3dc29a1556eee781e4a4c3a1f7f53f90fa834de098c4d
• target node(namehash('foo.eth') ) = 0xde9b09fd7c5f901e23a3f19fecc54828e9c848539801e86591bd9801b019f84f

Contract Result

Quote @Arachnid there're three possible issues:

1. You're passing the wrong ENS address to the FIFSRegistrar constructor

ENS address is correct, otherwise getRootOwner would fail to fetch the correct owner.

2. You're passing the wrong root node

Root node is fine given the following log screenshot

3. The name is already owned by another account.

Checked the target in ENS, confirmed it was not taking.

It's quite likely not the FIFSRegistrar or ENS issues, but the answer to this issue could eventually facilitate the ENS deployment and config process.

Decide on the next-step Chris can take to begin developing the playbook.

I believe invalidateName pays 50% to the previous owner, I didnt test but just wanted to get it out here before I forget
https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L449-L450
since the deed owner is not yet set to msg.sender, when you call setBalance(uint newValue) with 0.5 of the value the rest seems like going back to the previous owner
https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L70

I think these two needs to switch
https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L450-L451

startAuction and startAuctions throw if the auction was already in progress. This results in a race condition, where if two people open the same auction, one selected at random will get an error.

Instead, we should silently do nothing if the auction was already running, and only throw if the name is in a different state (owned, forbidden, reveal).

Such as IPFS, Swarm, MX.

Primarily for reference purposes.

Issue to track Piper's audit progress.

Dear contributors,

I am quite a noob in ethereum world, but still an engineer and can't find a solution to that problem online.

Following https://github.com/ethereum/ens/wiki/Registering-a-name-with-the-auction-registrar, I'm just stuck at the second step (check availability). The console tells me :
Error: invalid address at web3.js:3887:15

I have created some accounts, tried to set web3.eth.defaultAccount and unlocked it.

What I typed in the console is pretty basic:

loadScript('/path/to/ensutils.js'); // -> true
ethRegistrar.entries(web3.sha3('name'))[0] // -> Invalid address

Tried to add {from: ...}, doesn't change anything.

It is supposed that the bid amount is difficult to guess as SHA-3 is a preimage-resistant function. However, note that the hash input consists of

• the domain hash name, which is public;
• an Ethereum address, which is easy to enumerate;
• the bid value, which has low entropy if, for example, the user pays in integer amount of Ether;
• salt value, which has low entropy if the user does not have a secure random number generator (RNG).

Thus to truly conceal the bid value, the bidder must use a secure RNG to fill the salt value or use a high-entropy number of Wei as a bid. Such RNGs are not widely available to regular users, possibly except for password/SSH key generators. These high-entropy values must be secretly stored by a bidder to unseal the bid, otherwise the bid will be lost. Note that a repetition of either bid value or salt in two different bids would hint the attacker to predict these values for future bids from the same owner.

Note that salt in many contexts such as password hashing means a non-secret nonce rather than a secret random value.

A proper way to ensure bid secrecy without extra randomness would be to use deterministic encryption.

@Arachnid Could you please either -

• Tag the launch-dependent pending Issues and Pull Requests with the newly created "launch dependency tag"

or

• Let me know if there's an easy way to ID these, so I can tag them

Most of the .sol files are named after smart contracts defined in them, but there are several exceptions:

1. ResolverInterface.sol (defines AbstractPublicResolver smart contract)
2. interface.sol (defines AbstractENS smart contract)
3. HashRegistrarSimplified.sol (defines Deed and Registrar smart contracts)

This inconsistency makes it harder to navigate through the come and makes code reviews less efficient which in turn may lead to missed bugs.

@Arachnid Please send me your current to-do list. It's the list you referenced in our conversation earlier this week.

It seems that AbstractPublicResolver smart contract is never user. Also this smart contract looks very odd in general.

Need to update this section -

GET STARTED USING ENS

ENS launches on the Ethereum mainnet on March 14 2017. In the meantime, you can register names on the test network, Ropsten, and learn about how to integrate ENS support into your applications, sites, and DApps.

After an auction has ended, a third party can call startAuction again, restarting the auction process and zeroing out the values (but not refunding the winning bidder!)

The percentage of bid value, the bidder receives for unsealing valid non-winning bid (currently 99.5%) is hardcoded in multiple places:

1. https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L339
2. https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L339
3. https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L339
4. https://github.com/ethereum/ens/blob/master/HashRegistrarSimplified.sol#L339

This makes changing this constant very error-prone.