New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HashRegistrarSimplified.sol: Would it be possible to win a bid without depositing sufficient ether? #49

Closed
swaldman opened this Issue Mar 14, 2017 · 3 comments

Comments

Projects
None yet
4 participants
@swaldman

swaldman commented Mar 14, 2017

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.

@koeppelmann

This comment has been minimized.

Show comment
Hide comment
@koeppelmann

koeppelmann Mar 14, 2017

@alexvandesande just confirmed this bug in the Gitter channel - great catch @swaldman!

koeppelmann commented Mar 14, 2017

@alexvandesande just confirmed this bug in the Gitter channel - great catch @swaldman!

@MandarKarhade

This comment has been minimized.

Show comment
Hide comment
@MandarKarhade

MandarKarhade Mar 14, 2017

Great catch !! Wonderful work from community helps us grow !

MandarKarhade commented Mar 14, 2017

Great catch !! Wonderful work from community helps us grow !

@Arachnid

This comment has been minimized.

Show comment
Hide comment
@Arachnid

Arachnid Mar 15, 2017

Member

Thank you for spotting this. I didn't get a chance to update the bug last night, but I was able to reproduce it with a unittest more or less immediately, and once I did so, contacted the other keyholders to postpone the launch. We'll fix this bug - obviously - but also back off and improve our testing processes, write a postmortem, then reschedule a launch.

Thank you again for finding this before it became a major issue.

Member

Arachnid commented Mar 15, 2017

Thank you for spotting this. I didn't get a chance to update the bug last night, but I was able to reproduce it with a unittest more or less immediately, and once I did so, contacted the other keyholders to postpone the launch. We'll fix this bug - obviously - but also back off and improve our testing processes, write a postmortem, then reschedule a launch.

Thank you again for finding this before it became a major issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment