fix: set optimal default value for auctionReservePrice

[JesseMorningstar]

In the current deployment scripts for the nounish auction contracts the value for the state variable “auctionReservePrice” is set to a default value of 1 wei. This value leaves a lot of room for human error and has been recently exploited to settle multiple auctions with small bids of 1 wei. This vulnerability has not only caused loss of revenue to some DAOs that relied on affected deployment scripts, but it has also potentially left them vulnerable to sybil attacks launched with the governance tokens that have thus been accumulated virtually for free.

TL;DR:

When we set a default value that will be customized by the user, there are only two outcomes:

1. Either the user will adjust it up because our default value is lower than the desired value
2. Or they'll adjust it down because it was higher than the desired value
   Any default value that requires devs to adjust the value up will introduce security vulnerabilities given that they can forget to make the adjustment or they can adjust it up to a value that is still incorrect given that they're starting with a low reference point, and as a catastrophic result the auction will settle at undesired prices. Asking devs to input the value even with a madatory prompt is equivalent to adjusting up from 0. It is thus a fragile fix.
   Given that reserve prices build up a treasury there is no such thing as a reserve price that is too high. Therefore the only robust and systematic solution is to set a high default value for the reserve price and require devs to adjust it down if need be as this gives them a desirable and secure reference point as well as a verified channel (the governance process) with which they can evaluate whether or not they set the correct value. And if they forget to make the initial adjustment to the high default value attackers cannot take advantage of this oversight given the high cost for settling the misconfigured auction. This implements a security model of defense through deterrence. An even more desirable aspect of this approach is that setting a correct value for the reserve price will be non-optional given a high default value.

This second approach is therefore the only secure and guaranteed way to make updating the reserve price a non-optional configuration implemented with minimal code change.

In addition to being the most secure solution, requiring devs to adjust down the default reserve price creates very strategic opportunities for quickly bootstrapping treasuries as it will make every first auction of a new Nouns extension behave as a Dutch auction. An adjusting down outcome has therefore the major bonus of also making the Nouns protocol more successful in its stated mission of building an infrastructure that enables communities to bootstrap treasury.

Let us begin the festivities.

MOTIVATION

A pull request proposing to raise this default value to 0.01 Ether was submitted earlier.
This is an adjust up outcome that introduces potential vulnerabilities. After a thorough review of that pull request I feel compelled to submit a new pull request that takes a strategic departure from the solution proposed in that previous pull request to provide a more robust solution to the set of problems tied to this vulnerability, as well as a cleaner syntax that adds greater clarity and readability to the statement setting the value for "auctionReservePrice"

This approach implements an adjust down strategy that is systematically secure.
Here’s the rationale for this best default value proposed in the following pull request:

👉 Given that there is a large dispersion in the value of “auctionReservePrice” that future DAOs will implement, the probability that such DAOs end up failing to properly initialize this state variable to a value that is optimal for their community is very high given a default of 0.01 Ether, or given any arbitrary values, even one provided by the developers themselves before deployment. Therefore this commit defines a universal best default value that completely eliminates such a failure while leaving no room whatsoever for human error.

👉 And given that the underlying cause of the vulnerability we are presently patching is that a default value for "auctionReservePrice" was set using a naïve approach, this commit therefore provides a new default value characterized by a higher degree of bayesian sophistication because it is derived from a set of equations powered by game theory to inform optimal reserve price selection in an adversarial bidding environment.

As a result, this commit sets the new default value for the state variable “auctionReservePrice” at 68 Ether.

👉 Even if a default value of 68ETH is not optimal for a given DAO it turns out that it does protect the community from a failure to set the best value for "auctionReservePrice" since it establishes a high reserve price that ensures that the first auction cannot be won unless the correct adjusted reserve price has been set. Thus the proposed best default value serves as a forcing function that ensures that every DAO implementing this code must set the default value for "auctionReservePrice" to one that is optimal for their particular community.

👉 Most notably, this default value will allow the first auction of all future DAOs to work like a dutch auction: an auction starting with a relatively high reserve price that is adjusted down if the community so chooses. This will help Nouns fulfil its mission of providing a powerful infrastructure that helps communities bootstrap their treasuries. It must be noted that the opportunity to maximize the revenue of all future DOAs with one simple adjustable parameter, strong security guarantees, and zero side effects is just too great to pass up.

Therefore, I submit this PR for review and eagerly anticipate feedback from the community.

ACKNOWLEDGEMENTS

I wish to end this PR on a high note by highlighting the many conscientious and industrious individuals in ecosystem who submitted ideas, feedback, and code that has been integrated in this fix. So here’s a credit list to applaud each contributor’s efforts in noticing, discussing, and drafting a patch for this vulnerability, all with admirable expediency:

• https://github.com/sktbrd
• https://github.com/0xMillz
• https://github.com/bagelface

Long live the nounish buidlers ✨

[bagelface]

This is not proper way to force users to configure this value. If that is the objective, the parameter should just be made non-optional. Changing the reserve bid to 68 ether by default essentially guarantees an update will need to be made for deployers that overlook the need to specify this setting. Further, it is inconsistent with the minimum bid value specified in the webapp

nouns-monorepo/packages/nouns-webapp/src/components/Bid/index.tsx

Line 35 in 61d2b50

return '0.01';

[r4topunk]

Given the issues with the current default value of the 'auctionReservePrice' state variable and the proposed solutions, wouldn't it be more effective and safer to make this environment variable a mandatory configuration, rather than setting an extremely high default value like 68 Ether, which might still lead to human errors if implementers overlook the need for an update?

[JesseMorningstar]

The core idea here was to analyze our assumptions:

For example the previous PR was made with the assumptions that future Nouns extension will remain small.
But that is unlikely to be the case. And even if future DAOs NFTs don't sell for a lot, their reserve price will overwhelmingly be above 0.01ETH.

So a low value of 0.01ETH will in any case cause 98% of DAOs to update that value.
So a default value of 0.01ETH still requires an update to auctionReservePrice but it leaves them vulnerable to human error.
If they're going to update it anyway, then it's better to make sure that we have a value that makes it obvious that they need to update it.

Also, the first NFT of any DAO often has a lot of significance, so it's highly likely that the first NFT of a well-designed Nouns extension can fetch far more than 68ETH. We account for this and envision a scenario where the reserve price can be set back to something else after the first auction settles.

The real goal is to not just force them to change it, but in case they don't change it, the serendipitous event could put enough ETH in their treasury to give them a flying start.

And finally, good defensive coding requires a function to fail on incorrect input: a value of 0.01ETH can still be incorrect input but it will not cause the function to fail. But a value of 68ETH can be incorrect input that will guarantee that the function/feature fails for the end user, and devs can then catch that gracefully and set the optimal value without any damage to the DAO.

The key assumption here is that I'm not assuming that future DAOs can't sell NFTs for more than 60ETH.
I feel the assumption that Nouns extension have to be small is so engrained in us that we ignore the real potential for growth such DAOs have. From Gnars to FOODNOUNS these can be multibillion dollar extensions with the right focus and dedication.
There's no reason why bigger DAOs can't emerge in the space with NFTs that sell well over 100ETH.

And there is actually no inconsistency with the minimum bid.
I saw that and I made a deliberate decision because many community members signal their belief in Nouns by placing small bids on auction just to be part of the culture. This fix still leaves them the ability to do that.

The minimum bid and the reserve price are fundamentally two different things. IMHO there is no inconsistency there.

I really appreciate your feedback. Are there anymore assumptions I should elaborate on or clarify?

PS: @bagelface and @gustavokuhl does this bring more clarity and address your questions?

[bagelface]

@JesseMorningstar I appreciate the depth of discussion you're going into with this, but I think these changes are not as deep as you're suggesting. The reserve price and the first, minimum bid ARE effectively the same thing and should be the same value. A reserve price being the lowest price an auction can end on for it to be considered valid.

Your change would enforce 68 ether minimum for each auction at the contract level if it is not explicitly set at a different value when deploying the contracts.

nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouse.sol

Line 109 in 61d2b50

require(msg.value >= reservePrice, 'Must send at least reservePrice');

It's simply not a practical number in this case. I agree that it probably makes sense to simply require this value to be explicitly set, but that is not the intention of the other pull request. The intention is to patch a vulnerability quickly and simply. I think this PR is sort of missing the point.

[JesseMorningstar]

@JesseMorningstar I appreciate the depth of discussion you're going into with this, but I think these changes are not as deep as you're suggesting. The reserve price and the first, minimum bid ARE effectively the same thing and should be the same value. A reserve price being the lowest price an auction can end on for it to be considered valid.

Your change would enforce 68 ether minimum for each auction at the contract level if it is not explicitly set at a different value when deploying the contracts.

nouns-monorepo/packages/nouns-contracts/contracts/NounsAuctionHouse.sol

Line 109 in 61d2b50

require(msg.value >= reservePrice, 'Must send at least reservePrice');

It's simply not a practical number in this case. I agree that it probably makes sense to simply require this value to be explicitly set, but that is not the intention of the other pull request. The intention is to patch a vulnerability quickly and simply. I think this PR is sort of missing the point.

I linked in a paper that details the extreme importance of the reserve price.
The point of the reserve price is to build up a treasury.
In Lil Nouns there has been a healthy discussion on whether they should lower the reserve price, and we can actually draw from that recent data to see how much of a strategic asset the reserve price is for a community.

I have to emphatically state that the reserve price is not the same thing as the minimum bid.
A a reserve price is the "minimum valuation" of an asset.
The minimum big is the "minimum value for starting an auction".

These are wildly different things.
Making the reserve price the same as the minimum bid is very ill-advised and just make no sense whatsoever: why would you value any asset at the minimum price needed to start an auction? Doesn't that defeat the point of the whole exercise?

It's precisely because the scope of my PR is fundamentally different than the previous PR that I didn't just suggest syntactical changes to it. I'm submitting this to highlight that we need to futureproof these scripts given that there is a very high dispersion in the actual reserve prices of Nouns extensions. This statistical dispersion will lead to human error and guarantee that something like this happens again.

Current data shows this, and the trend in this dispersion will only grow as Nouns become more popular.
Even with current data most Nouns extension have a reserve price above 0.01ETH, if the point is to prevent something like this from happening again, a reserve price of 0.01ETH absolutely does nothing to achieve that goal.

Also calling 68ETH impractical could be a fallacy known as argument from incredulity. Statistically speaking 68ETH is actually not impractical.
I used actual auction data from Nouns DAO to come to that value using some formulas indicated in the paper.

Let's think about this, if Nouns extensions are supposed to be at least as good if not better than Nouns DAO, then the set of assumptions we should have indicates that 68ETH is actually rooted in data and not impractical at all. I recommend you look at the trend in closing prices of the first auctions that settled for Nouns DAO, and you'll see a pattern.

Also, this is not a trivial matter.
It's about creating a codebase that really fulfills the purpose of Nouns: to help DAOs bootstrap treasury. If we fail at that, Nouns as a protocol has failed. We can in fact create a good metric for whether Nouns is succeeding on this front of its core mission by tracking the number of extensions whose auctions regularly close near or above the value I suggested. My projection, and you'll admit the projection of anybody who believes in the power of proliferation, is that over time such a number will grow.

I really appreciate you for being a good sport and taking the time to provide a very constructive critique of this PR.
And oh, and before I forget, do read that paper I linked to if you want to get down to the mathematics of why reserve prices matter so much in maximizing the expected revenue of first price auctions such as English auctions.
It'll be well worth the effort. 👌

[r4topunk]

I understand @JesseMorningstar intention of adding a high value to prevent the operation due to the excessive amount, which will require the developer of the fork to search and modify the code to complete the fork. Therefore, I reiterate, the best option is to have this in an environment variable or a constant, which can have a default value of 0, and a validation that checks if the value is zero and terminates the program execution. I also think it doesn't make sense to change the same information in multiple places when it's the same information across different files...
To me, the optimal default value is 0 in a centralized file (const or env), with a validation to verify if it's not 0.
What do you think @bagelface?

[bagelface]

I understand @JesseMorningstar intention of adding a high value to prevent the operation due to the excessive amount, which will require the developer of the fork to search and modify the code to complete the fork. Therefore, I reiterate, the best option is to have this in an environment variable or a constant, which can have a default value of 0, and a validation that checks if the value is zero and terminates the program execution. I also think it doesn't make sense to change the same information in multiple places when it's the same information across different files... To me, the optimal default value is 0 in a centralized file (const or env), with a validation to verify if it's not 0. What do you think @bagelface?

Yes. We are all on the same page that a non-optional value is the best solution. However, the PR I created is a quick and simple solution that is effective. If someone is able to make a PR that makes this value non-optional in a way that does not negatively affect general usability of the scripts (which is this case with setting the default value to 68 ether), I will happily close my PR and endorse one that forces a user to explicitly set the value. However, in the absence of that PR, we need a quick fix to prevent others from accidentally deploying an auction house contract with effectively no reserve price.

[JesseMorningstar]

@gustavokuhl I appreciate the understanding. I also thought about ENV variables and decided against it for the following reason:

1. We should not encourage devs to copy pasta any code because then they make more assumptions that will most definitely be proven wrong over time. They should actually be familiar with the codebase they're working with otherwise they are guaranteed to make some other mistake or oversight like this in the process of deployment.

2. They will have to go through multiple files anyways to configure the other parameters. So if we set this one parameter in ENV variables we should set all of them as well. And that is actually impractical in this case.

3. I've started working on a Nouns Configurator UI where devs can input all the parameters they want to produce the mono-repo they will deploy. It would be nice to make all this a point-and-click experience to remove all requirement of technical skills in the process of forking the codebase. This UI will add many niceties like check lists, securities recommendations, etc. It can even have an automated walk-through that actually helps new dev quickly get up to speed with the codebase.

4. Also, all of us should band together to begin to teach other devs about this codebase. We need more people intimately familiar with this marvel.

It's for all these reasons and more that I did not opt for the ENV variables solution.
With that being said, it's terrific thinking on your part ser! 🤝

[JesseMorningstar]

However, in the absence of that PR, we need a quick fix to prevent others from accidentally deploying an auction house contract with effectively no reserve price.

A reserve price of 0.01ETH is effectively no reserve price given that it is equivalent to the minimum bid of an auction.
This fact should be abundantly clear to any astute observer. It's simple math sir.

And that is the core problem with the previous PR: it really fails to differentiate the minimum bid from the reserve price and as a result sets a default value that ultimately fixes nothing at best, but in reality adds unnecessary logical redundancy to the code.

[bagelface]

However, in the absence of that PR, we need a quick fix to prevent others from accidentally deploying an auction house contract with effectively no reserve price.

A reserve price of 0.01ETH is effectively no reserve price given that it is equivalent to the minimum bid of an auction. This fact should be abundantly clear to any astute observer. It's simple math sir.

And that is the core problem with the previous PR: it really fails to differentiate the minimum bid from the reserve price and as a result sets a default value that ultimately fixes nothing at best, but in reality adds unnecessary logical redundancy to the code.

You're completely missing the point of my PR, which is to make the scripts safer for users who are not explicitly setting values. I'm not here to argue semantics with you, I'm here to make practical improvements to the code base in a timely manner. My pull request does that, yours does not.

A reserve price exists to ensure a minimum value is met for an auction. It is necessary for the first bid of an auction to be AT LEAST the reserve price, therefore the reserve price and minimum value of the first bid of a given auction are the same value. The frontend is currently designed in a way that implies the minimum first bid of an auction must be 0.01 ether. However, that is not the case at the contract level, which is where the issue manifests. An alternative fix would be to update the minimum bid as it is displayed on the frontend to be 1 wei. However, it is my understanding that most DAOs do not actually want the reserve price to be 1 wei. They were just under the false impression that it was 0.01 ether. My PR resolves this misunderstanding by making the two values consistent. Your (this) PR only adds further complexity to the usability of this repo.

I'm done arguing this point. As far as I'm concerned there is nothing more to be said. Until a PR is made with updates that make this parameter non-optional, do not expect another response from me.

[JesseMorningstar]

A reserve price exists to ensure a minimum value is met for an auction.

That is spectacularly wrong.
A reserve price exists to maximize the revenue to the seller.
And that my friend is the point that I'm trying to convey to you: you fail to understand what a reserve price is.
We're not arguing, we're all friends here having a lively intellectual discussion.

The difference between minimum bid and reserve price is not semantics, it's finance.
There is nothing more consequential in designing an auction than setting the correct reserve price, and that is science as described in the paper I shared with you.

It's really simple: while a minimum bid exists to prevent spam, a reserve price exists to MAXIMIZE the revenue that an auction produces, which means it helps build a treasury up. The paper I linked goes in-depth on details of just how big of a positive impact setting a high reserve price has on revenue. Have you read it?

If you haven't take a good look in the mirror and ask yourself this: are you really debating in good faith?

I've presented all the evidence to indicate why this PR qualitatively solves the full set of problems related to the auction reserve price. I really hope we've checked our egos at the door and are genuinely seeking the best solution to this problem.

The fact of the matter is and remains that you simply can't be successful building up a treasury if you make the minimum bid on every auction equivalent to the reserve price. This is simple arithmetic ser.

So a reserve price of 0.01 ETH is not desirable as it does not give a DAO a chance to build up its treasury adequately.
This is exactly why most treasuries set their reserve price far above 0.01ETH.

So this means DAOs will definitely have to change that value.
If they forget to do it, they're screwed.
Your PR fails to address this simple statistical fact and therefore guarantees that this exploit will happen again.

With the present PR there is NO room for human error given that if the devs forget to set the value at deploy time worst case scenario is they instantly realize it on the first auction, likely scenario is someone actually bids that reserve price and they have at least a nice 68ETH to bootstrap their treasury. That is they've maximized the revenue on that first crucial auction.

This scenario fulfils the stated purpose of Nouns' codebase and is only possible under this PR.

The point is that with this PR there simply is no way a community gets a negative outcome out of this code, in fact it sets the stage for a huge win for any DAO that may happen to forget to change the code.
But the previous PR you submitted statistically guarantees that a DAO will face a negative outcome given that 100% of DAOs desire a reserve prices above 0.01ETH in order to build their treasury.

I appreciate your passion. And I must say that our exchange was constructive.
All of this is to me just the first step in a process of optimizing this code base.
I hope we can keep the lines of communication opened so that we can do terrific work together on this repo. 🤝

[sktbrd]

Non optional is the best option

[JesseMorningstar]

Non optional is the best option

Indeed, and the only way to implement that is to set a high default value that the devs have to change if it doesn't suit them.
That is the exact core idea of the PR.

I appreciate you for chiming in sir.

[bagelface]

Non optional is the best option

Indeed, and the only way to implement that is to set a high default value that the devs have to change if it doesn't suit them. That is the exact core idea of the PR.

I appreciate you for chiming in sir.

See my alternative PR to require the explicit setting of this value when running the task. #831

[JesseMorningstar]

Non optional is the best option

Indeed, and the only way to implement that is to set a high default value that the devs have to change if it doesn't suit them. That is the exact core idea of the PR.
I appreciate you for chiming in sir.

See my alternative PR to require the explicit setting of this value when running the task. #831

Yes, I saw that that. It's neat.
I personally did not go for that because I believe that the cure must not be worse than the disease: changing inputs to the build in such a way would require the maintainers to test everything to ensure that nothing else is going to break downstream.
But even worse, it still leaves room for human error, what if they miss a zero? what if they don't do the wei conversion correctly? What if they put a completely wrong value?

Also when you start changing what the inputs to some a large code base are, you may introduce new vulnerabilities.
I sought to create a PR with no side-effects. So it's up to maintainers to see if they're comfortable with that level of risk.

Also as you'll see in the days and weeks ahead this PR sets the stage for actually creating a protocol that helps raise the revenue for Nouns extensions. There is simply no such thing as a reserve price that is too high.
Once that's understood we can see lots of strategic opportunities in this PR with minimal changes that open up massive opportunities to grow the ecosystem in impactful ways.

I guess it all comes down to product development vision now.

Btw I must say that I really, really like your style: you're a doer.
Doers are my favorite people 🙌

[0xMillz]

I'm a little late to the discussion, my apologies. Rather than try to respond to everything said here in human language, I hope you don't mind if I submit own PR as my response (along with a brief tl;dr). I think it would be the clearest way for me to show my agreements, some disagreements, and also my own ideas of how a change like this could best be solved.

I appreciate the professionalism here--OSS can be tough--and no offense taken if you all come to a solution before my PR code is pushed, as it is getting late in my TZ so I can't guarantee it'll be up tonight.

Best,
Millz

[JesseMorningstar]

I hope you don't mind if I submit own PR as my response (along with a brief tl;dr)

The more the merrier!
I strongly encourage you to take your time, maybe we've missed something worth highlighting.
I personally ask maintainers to not consider this PR until they've fully vetted all other PRs tackling the same issue.
There should be no PR left behind.
This is not an emergency. We can take the time to do this and do it well.

All affected communities have already put out proposals to call setReservePrice so this gives us the time to really get to the bottom of this very interesting issue, and do it from all relevant angles.

The issue of reserve price seems simple but it actually is very consequential so I think that everyone who has anything worth saying or adding to this discussion should create a PR and present their solution in the only form that matters: code.

And let us all remember that how we do the small things is how we do everything.
So let' not mail it in on this one, let's go the extra mile.