kleros/stake-curate

Curate with indefinite, capital-efficient stake.

stake-curate's Issues

Make items compatible with EIP-721

This would allow explorers such as Etherscan to show that these items exist.
There's no need to implement a tokenURI, as that would introduce edge cases. But, a general one could be made, or maybe a general one per list.
Probably, it would need to implement ERC-721 Enumerable, along with other stuff.
Since Stake Curate takes place in a rollup, the extra gas overhead may not be significant, as long as the EIP-721 doesn't leak into creating more calldata for the transactions.
The L2 gas will increase a lot, as now it would need to keep track of extra information such as how many items are owned per user.

The transfer functions will probably need to be reverted, as owning items is a liability.

List extra flags ideas

There's plenty of space (64 bits) for more settings in every List struct. Some ideas for booleans:

1. ban harddata
2. force harddata
3. ban edits
4. ban ipfs
5. force ipfs
6. allow adoptions by default (still requires unchallenged item and enough freeStake)

still 58 bits that could be put to use, any suggestion?

Submitting evidence should point to an item slot

Figure out the evidenceGroupId in the method, pointing at the item slot

Make tests for Cint32.sol

Make a test suite for Cint32.sol
Mostly, check that a list of numbers stay within 0.1% after compress -> decompress., and are always larger than the result. some numbers that may give problems:

• 0
• 2^24
• 2^32
• 2^255

try for numbers immediately above and below them. and test for a bunch of random numbers of different scales.

MetaEvidence is wrong

Each list must have its own MetaEvidence

Remove ArbitratorExtraDataId in DisputeSlot struct

making arbitratorExtraDataId 64 bits instead of 32 meant that DisputeSlot struct needs more slots, which is awkward.
turns out that the aparent sole reason we stored arbitratorExtraDataId in DisputeSlot was to make appeals. But stake-curate doesn't bother with that anymore, so it can be removed

add tests for the failure cases in editItem

Turning on Commit Reveal

While the current way things work is sound (frontrun strategies are a net loss), much of the contract got uglier to cover the edge cases.

Samples of this ugliness:

• Adding % burns.
• Allowing flash withdrawals, forcing them to pay the burn cost.
• Forcing a minimum challenger stake ratio, forcing any challenge to burn stakes in some way.
• Adding an editionTimestamp to challengeItem, so that an specific Edition can be targeted (to cover against Edit baits)
• Adding reason, to prevent a challenge to frontrun an Edit without yet knowing why or if it's incorrect (this is healthy, though, and will be kept)
• Implying that legit challengers may have to watermark their reason ipfs files in some way to protect against frontrunning.
• Implying that legit challengers will have to play a less-than-zero game, baiting frontrunners that mirror challenges
• Implying that, if a AI powered MEV solution was built, they would eventually be able to read an incoming challenge, redo the reason, figure out validity, etc...

---

If commit reveal were to be triggered on, at the very least it should be used for challenges.

Edits and recommits might need commit reveal due to frontrunning instances like:

• Item owner sees impending proof of a challenge
• Frontruns an edit.
• What happens now, should challenge revert? should it succeed and be made on the basis of the challenge that was committed? (so, keeping editionTimestamp as an argument, as it currently works?)

If editionTimestamp is kept (which, I think is desirable), then only challenges will need commit reveal.

Consider discarding items rewrittable storage

This is a hard call to make but I wanted to write it somewhere so that I can reflect on it later.
Currently, stake-curate has rewrittable item storage. This means that creating new items should be significantly cheaper if you're choosing used slots. More even so taking into account the saving is in L2 gas, since stake-curate will live in a rollup.

Recently, a decision involved allowing to write arbitrary data as bytes in the items (#17). In order to make it easier for other contracts to interface with stake-curate, the id of an item should be predictable.
This is not compatible with rewrittable storage.

we can either approach it as "not our problem" and keep the position of the items unpredictable, and force proxies to deal with that complexity. This will mean significantly cheaper item submissions in reused slots.
(It's not that hard to deal with that complexity, it's just annoying. proxies would have to deal with timestamps, etc. But they probably would need that anyway.)

Or we can dump the rewrittable storage idea entirely. This makes reading data onchain easier. It also simplifies a few methods. It also reduces some complexity off the user, and eliminates the need for the user to understand the slot system. Two routes:

• keep an itemCount. this will mean having an extra storage read and storage write per item creation. straightforward, but expensive. Simplifies a few things.
• allow all slots to be taken in any order. only rule is, a slot cannot be rewritten even if the item has gone through its lifecycle. This is cheaper but keeps some complexity (frontrunners could take a slot intentionally)

This is going to be a hard call so I'll take my time figuring out what better for the project

Use custom errors instead of require

They're cheaper, apparently
require with string is way more expensive on deployment

support multiple arbitrators

stake-curate has this one problem: currently it's using one hardcoded arbitrator
in the event that an arbitrator turned out to be deprecated, that's dangerous. because newer arbitrators can mean that old arbitrators get less eyes put on them, or less support.
as long as an arbitrator supports the arbitrator interface, it should be supported.

stake-curate cannot be easily redeployed as a contract! that's because, all the users would have to manually migrate their items onto the new one, withdrawing the deposit, etc

work on this issue after #3

changes:

• create new struct, "arbitrationSettings". it stores arbitrator and arbitrator extradata. must be indexed ny 64 bits.
• arbitrator extradata is no longer a thing by itself, remove it from everything.
• fit arbitrationSetting onto the list struct.
• allow update list to change these arbitrationSettings

Governor as account in List struct

arbitratorExtraDataId is too short and it allows for attacks (since it's 32 bits), but there are currently no bits to spare
Governors could just be Accounts too, making address take 64 bits instead of 160. Now there's space
however, now you can't make a governor be address zero, if you wanted to make sure no one can be governor. because you cannot create an account from address zero.
a way around this is to, on contract construction, create the Account 0 as address 0x0 with 0 funds. so that everyone can easily make immutable lists
(it's not needed since someone could just make a contract that can only call the createAccount function, and thus have a "null governor", but it's prettier that way)

Document and deal with Badges at subgraph level

Badges should be handled at subgraph level for speedy queries.
The architecture is:

Main list (linked in MetaList with Badges list)
Badges list (linked in MetaList with Main list. List of Lists)

• Badge A (linked in MetaList with Badges list. Each Item is a reference to an item in Main list.)
• Badge B (linked in MetaList with Badges list. Each Item is a reference to an item in Main list.)
• (...)

Edit functionality

Current Curate doesn't have edit functionality and it's just lame. Stake Curate should make an effort to get it before it's too late

The scope of this issue is simply to add the edit method to the smart contract. Read my notes below and figure out what does the editItem method need, how does it work, etc.
it should emit an event like ItemEdited that broadcasts the new ipfs uri.

it goes without saying that the method must be safe

No need to check for overflows

Add unchecked to a bunch of places
there no need to check for overflows, I think? I wonder if theres such a need anywhere

Update liveSince when item is Disputed

You should not necessarily update it
I was wondering whether if this was an edge case, but, since Disputed items cannot be challenged, the item cannot be challenged right away, so this will only become an issue if the item is edited just right before the ruling is received. Writing here just to track it as a non-issue.

Remove forListVersion from item updates?

With account abstraction, you could batch transactions and revert if listVersion has changed.

Without account abstraction, removing this would be a security hazard, since list owners could frontrun an update to the list and change the policy in a way that's destructive to the submitter. So, that's the rationale. Just putting it as an issue to let it be documented.

Add "prefill" query parameter

The idea is to facilitate shipping decent-UX PoCs that are fast to build.
Some use cases (examples below) could really benefit from this.
The gist of it is: the query parameter(s) can be used to prefill some forms in the frontend, to make the experience seamless for those apps that didn't get time to fully implement Stake Curate functionality, like signing Stake Curate transactions, which is non-trivial (due to complexities like editions, etc)
This could also be a deliberate decision. e.g. app lives on a different chain than Stake Curate, app doesn't want to handle the complexity, etc.

Examples

Social Media App

User is in curated social media app. User reads a post that doesn't belong to a certain topic (e.g. contains spam). User clicks the tag in the post, because that's how you challenge. A text field pops right below, to prompt the user into writing the reason for the challenge. Like, quoting one of the rules of the topic.
After filling the stuff and clicking "Challenge in Stake Curate", the reason (this is, the link/identifier to the offender post, and the rule that is broken) is passed as a query parameter. The user finds himself in Stake Curate frontend, but all he has to do is connect to the Stake Curate chain if he hadn't yet, and click on "Challenge".

Whitelist Telegram

Similar flow. User sees offending message in group, that infringes one of the badges. User types
/challenge #noscam rule 1
And the bot replies with the link to Stake Curate frontend that automatically challenges the item (in this case, a badge) and prefills the reason.

This is not ideal (in this example, any chat user could see the challenge intent and frontrun it to snatch the reward)

Not just reason

This is not restricted to reasons. Prefill query parameters can be used to fill arbitrary fields to submit / edit items, for example.

Notify if the MetaEvidence options have been tampered with

The flow will be something like this:
-> arbitrator sees thing, and clicks on a link on the injected evidence display which directs to stake curate default browser
-> stake curate frontend tells the user that hey, the ruling names are not what was expected, and it explains the three options and what they mean.
The problem with this is that the injected evidence display is also part of the MetaEvidence that can be tampered with, so not great. But at least users that use stake-curate have a chance to learn about this.

The only way to truly fix this issue is for MetaEvidence to only be submitted by stake-curate governor, and use something else instead of MetaEvidence for the List specific stuff. But then the jurors would not have access to the policy on a single click (but you can provide that policy just fine through the evidence display)

Good thing is, lists that are misgoverned with malicious MetaEvidence are worthless for stake-curate (they won't be curated in a list of lists with proper MetaEvidence). But they may hurt the jurors of an arbitrator like Kleros that struggle converging to the schelling point (should they vote according to the readable rulings, or they real meaning?)

Figure out how to deal with Property Types

Classic / Light Curate dealt with property types through a gtcr-encoder library.
Stake Curate could curate these property types on a curated list, or accept them as PRs.
So far, the subgraph does not interact with the types in any way. The type is just a tag for the frontend to interpret, and the value is always encoded as a string.

Ideas for new types:

Enum

MetaList will contain a property type such as Enum("Thing", "Stuff", "Car").
The property type on itself contains the possible options for the enum.
This will make the frontend render a selector on creation / edit.
On display, it shows if the value doesn't belong to the enum.

Rich address

Could very well be interpreted through CAIP-10. The acceptable addresses could be curated themselves, although it'd be simpler to add them in the form of PRs.

List

Just a number that references a List. The frontend displays a link to the List, labelled according to it's name or showing that's invalid (not existing yet, or incorrect name).
On submitting, it should autocomplete to show the fact that the list exists, etc.

Item

Just a number that references an Item. Same deal with List.

Add fullText queries to Subgraph

Stake Curate should be importing Cint32.sol

Right now it's using an outdated copy of compress and decompress

Send file uri as bytes32 instead of string?

Currently, stake-curate emits _fileUri according to the ethereum/EIPs#1497 that sends strings such as /ipfs/{hash}/{file}.
This is not efficient. Strings like this will usually take 4 32-byte slots of calldata, as they require to keep track of the length of the string, plus the string is long by itself, usually requiring 3 slots.
The Evidence standard will likely not be refactored any time soon. However, stake-curate off-chain item data handling doesn't necessarily require passing the _fileUri according to this standard, it could simply continue using ERC-1497 compliant strings for Evidence and MetaEvidence, but use a custom scheme for items.

The issue with this implementation is that, I haven't seen any examples of this in practice, and asking around IPFS I still got no guarantees it's even possible. Taking rollup cost reduction into account, this optimization may come in too early, so it could be kept around for a newer version. It is also possible that ERC-792 and ERC-1497 standards become refactored in the future for gas optimization.

If these standards worked like that, calldata would be cut by half, which may be the bottleneck in the next few years. So this should be kept in mind.

Slot view simplifications

Extend the Enum, to describe every state of Item "Limbo" (Removed, Disowned, Uncollaterized...) Many of these states won't be ever stored. Make a general view function to dynamically obtain this ItemState, that will significantly ease on-chain adoption. Use this new view function to remove a bunch of views that no longer will be needed.
This will likely increment gas expenses for regular interactions, but since Stake Curate lives on a rollup and it incurs no extra calldata to interact, it should be worth it.

Items could store arbitrary data

you could pass optional arbitrary data to adding an item or editing an item
it will be stored as a bytes
harddata or chaindata are potential names for this
the purpose is to (possibly) use stake curate for onchain storage (e.g. proof of humanity) for some use cases that need it, without having to create a separate contract to support it

Figuring out Item age on-chain

At the time of filling this issue, ageForInclusion is a MetaList property.
But to easily support on-chain Use Cases, this should also be on chain. Then, you would be able to check the age of an item, and if it's Young or not, on-chain.

Doing this highlights a current issue with balances and deposits. The following edge case:

• Alice submits Item on List with ageForInclusion = 3 days
• Alice immediately removes her deposit in some way. Examples:
  • by withdrawing, because she passed the withdrawalPeriod
  • by wrongly challenging something else and locking her deposit
• the aging period passes, and during which the Item could not be challenged.
• Alice gets more freeStake (by making a deposit or winning a challenge against her, for example)
• The Item suddenly appears to have overcome the period

Now, this is not necessarily an issue. Someone could look at the properties of the Item and, despite being undercollateralized, set a bot or something such that, the moment the account holds enough freeStake, challenge it.

But this assumes people will run these bots. Plus, it's still vulnerable to this attack: sandwich a function on another contract, that will now read the Item, conclude that it's Included, and cause harm.

A way to go around this issue, is keep track of all the balances that an account has. Then, you somehow find out that, during the period, the Account held less than the requiredStake.
Or you can even loop to the oldest, uninterrupted instance of having enough freeStake, to obtain the exact age.

But then this creates an issue I'm not sure how to go around. This is, in both instances, O(n), to loop from most recent balance to the one that either:

• doesn't have enough freeStake
• or is beyond the inquired period (that can go in two flavors)
  • either the timestamp of the item, to find out the exact age
  • or the ageForInclusion, to just find out if it's enough

O(n) complexity may not be good enough, not even in Optimistic Rollup. Because, this balance would have to update every time the freeStake changes:

• every withdraw
• every deposit
• every time Account suffers a challenge
• every time a challenge is resolved by Keep. (and, if it was desirable to track a complete balance with lockedStake, just, anytime it's resolved)

I don't know any data structure other than looping to solve this issue.
Just imagining that only freeStake is required, that turns it into an array of 64 bit FreeStakeUpdate structs (32 bit for freeStake, 32 bits for timestamp), that might reduce the O(n) cost of looping to "O(n/4)", at least in SLOAD cold terms.

---

If it was decided to not keep any of this on-chain, and ageForInclusion was dropped for on-chain use cases, then all this complexity can be outsourced to the subgraph. But if Stake Curate is aiming to become a truly generalized solution for curation, then it should go around this issue.

Some ideas to go around this:

(Although the most important thing is, to learn how many cold SLOADs can fit in a block, so that at least I can get guarantees of liveliness)

• Limit growth of this FreeStakeUpdate array in the following way: have a concept for "time segments". Say these time segments are 3 days long. Whenever a freeStake update occurs, the value is only changed if the new value would be lower. Then, it instead shows the "lowest freeStake amount that this Account had during this period". It also removes the need to keep track of the timestamp in the array. This means:
  • Cold SLOADs are n/8 now
  • Growth of this array is limited, so even if it's O(n), it should be slow enough for most ageForInclusion values. (Or, if we're interested in the exact age of the item, it still should be enough)

Flaws with Withdrawal and possible solutions

(this issue assumes a few other features and fixes have already been added)
The Withdrawal mechanic works, simplistically, in the following way:

1. Alice starts withdrawing startWithdrawAccount
2. some time passes withdrawalPeriod
3. the moment the period passes, her items are Uncollateralized until she withdraws
4. Alice chooses an arbitrary amount to withdraw with withdrawAccount, closing the period and allowing her to have Included items as well. She can withdraw as long as the amount doesn't exceed her freeStake

---

The issue is that, the way incentives are currently set, and assuming Alice could possibly frontrun anyone, there is no reason whatsoever to follow this path. She could withdraw her entire freeStake "instantly" (assuming account abstraction existed), or in a very short time frame.

The core way to do it is, she submits an Item to a List she controls. This is a proof of concept:

1. Alice creates a List with requiredStake = her freeStake, that uses an Arbitrator she controls
2. Alice submits an Item to the List, and challenges it.
3. Alice wins her own challenge and gets the stake out.

---

Stake Curate main purpose is, wrong Items shouldn't be in the Lists. This instant withdrawal doesn't necessarily defeat the purpose, but it introduces a frontrun rabbit hole that is not friendly to participants.

Frontrunners can also be frontrun, and it would get challengers and item owners into this arm race.

---

Solution A: Accept this

Remove the withdrawal period feature. Now you can withdraw any stake instantly.

Solution B: Burn stakes on challenge

Note: Whenever I use burn, it could also refer to sending the portion to a Stake Curate treasury.

Reduce the reward that a challenger gets on a successful challenge, burn a given %.
Reduce the reward that an item owner gets on a failed challenge, burn a given %.

• In order for this one to work, a minimum challengerStake must take place per list. Otherwise, challenging and failing the challenge (to lock the freeStake temporarily and revert a challenge) comes at no cost.

---

For both solutions, the minAmount mechanic to protect challenger from a frontrunner should be removed, and just revert when the Item is Uncollateralized.

These issues would also take place without allowing multiple Arbitrators, you would just have to create a List with extraData that costs the lowest amount possible. Just, the fee would go to the Arbitrator operator instead.

make function to create an account in the name of another address

its the same as createAccount but you pass the msg.sender instead of assuming its the msg.sender

why not just change createAccount

to allow users to save those 160 bits if they so desire

General Policy Review

0. a. "Author": I suggest "submitter" to keep in line with existing language. Also the actual author and the submitter could always be different people and this could be confusing.
   b. "MetaList": Should be named "ListMeta" instead, or better still perhaps, "ListMetadata".
   c. "Challenge: process in which the inclusion of an Item, in regards to an Edition, is put into question, and it creates a Dispute." If I've understood correctly, I suggest the following instead which should be a bit clearer: "Process by which an edition of an item is disputed. If the challenge succeeds, the Item is removed." (but is it the Item or the Edition?)

0'. You don't define when an item is to be considered registered. From what you've said previously I'm guessing this is up to the downstream dapps to decide but this is quite confusing to me.

0''. Instead of giving an unstructured list of definitions, I think an explanatory paragraph or two (where you can bold the keywords) would be more helpful.

1. "This document will refer to the General Policy as Policy from this point onwards." => "as the Policy" but you could remove this passage and just refer to it as "The General Policy".

2. "This document will refer to the General Policy as Policy from this point onwards. This also refers to all documents this Policy holds authority over." Second sentence is not clear at all. What's "this", and how can a document hold authority over other documents? Maybe you simply meant: "The combination of this document (the General Policy) and the List Policy of the specific list under consideration (as defined in the previous paragraph) will be referred to as the Policy (bold)."

3. Nitpick: You can refactor this: "The terms must, must not, shall, shall not, should, should not, may, required, recommended, not recommended and optional, have their meaning specified in RFC2119.

To ease understanding, the Policy may only use the following subset of terms: must, must not, should, should not and may."
=> "The terms must, must not, should, should not and may have their meaning specified in RFC2119. This applies to List Policies as well.".

3'. You say "the Policy may only use the following subset of terms" but what if I create a List Policy that uses other terms? Surely we don't want to consider it void in that case. Better not to insist on that point IMO.

4. "Upon creation of the Dispute, the Challenger must give a reason for removing the Item. It must be a valid reason according to the Policy. If this reason is not valid, the Arbitrator must rule to Keep the Item. This is the case even if the Item does not belong for any other reason, even if implied otherwise on the following clauses."
   a. Upon is vague. Must the full reason be given in the same transaction as the one creating the dispute? Personally, I think it's good to give the challenger some time to formulate their reasoning after challenging. Otherwise, if the reasoning is complex, this could lead to multiple challengers spending time and effort writing their justification in parallel while only the fastest one will get to reap the benefits. I suggest you allow 24 hours to submit a justification.
   b. "The reason should be clear and exhaustive. The reason must not be ambiguous or open to interpretation." I would add: "The reason must be reasonably specific (not e.g. "The policy does not abide by one of the rules.", or "The item is a duplicate of another item in the list." (without specifying the duplicate item))".
   c. Are parallel challenges possible? I don't see how they could be. Which means one can easily slip an item in by self-challenging with an invalid reason. Or perhaps an item is not considered to be part of the list until it has been unchallenged for a sufficient amount of time, but in that case, the item can be attacked so it never makes the list. I personally dislike the current challenge format as well, from a juror's perspective especially, since, in case no reason is given or the reason given is bad, jurors have to check every single detail of the submission, but there's no simple way around it.

5. "Required Props" If you mean properties, I'd say "properties" in full. Prop already has several very different meanings in English.

6. "An Item must be removed if it lacks Props whose Column is marked as required." => "if it lacks a Prop/Property"

7. Why "column" instead of "field"? I'm also confused by the data structures here. You say a Prop is a (label, value) pair. Do you mean (field name, field value) pair then? If so, I'd drop the Prop/Column language altogether. E.g. "Required Fields" "An Item must be removed if it lacks a field marked as required", "Intrusive Fields" (maybe "Unspecified Field" would be a better name) "An item must be removed if it contains a field unspecified in the List Policy" (not sure if that was your intended meaning).

8. Regarding "Frontrunning, baiting and Editions", I'm sure you've thought about it, but why not have the edition be part of the txdata? In practice, being allowed to push a new edition once the challenge transaction is in the mempool will definitely allow attacks. And relying on flashbots is not acceptable since this service is not guaranteed to remain accessible and otherwise relies on trust assumptions.
   I'm also confused: what happens if multiple editions are published in quick succession? Can only the last one be challenged? What happens to the previous ones? If the last edition is removed, is the item itself removed?

8'. TODO after you answer 8..

8''. Disregarding previous points: "the previous block in which the Challenge was created" ("in which" somewhat confusing) => "the block immediately preceding the one in which the Challenge was created (henceforth Previous Block and Challenge Block respectively)"

9. Not sure what the point of the "Challenge Cooldown" clause is since the item will still have "disputed" status. Is it just to lift the burden of stakers having to defend themselves too often?

Tests are interdependent and they should be isolated

Tests are interdependent. You should use something like beforeEach to isolate unit tests. Whether if you want this beforeEach called before each unit test, or before bundles of unit tests (e.g. accounts, lists), it's up to you.

Set arbitratorAndDisputeIdToDisputeSlot to zero after rule

Self explanatory, will trigger a gas refund, making it cheaper to call rule. This slot won't be used again

Make stakes compatible with EIP-20

Instead of using the native currency, just use ERC-20 compliant tokens for the stakes. Using standards allows for greater interoperability.

However, this will greatly increase complexity inside the contract. It will require approve calls from the frontend (although, with Account Abstraction, they should be gone). It will incur heavier gas usage. It will generate edge cases. It will mean storing, per account, a balance per ERC-20, which, for some "deflationary" (burn % per transfer) tokens will mean reverts and issues. Will render testing and auditing much more difficult.

Lists must now store the ERC-20 token they are utilizing, and it can be updated. As Items cannot be challenged after a List update, Items shouldn't need to store the ERC-20 they're utilizing.
Disputes must store the ERC-20 that was being used at the time of the challenge, otherwise, an attacker can update the List's ERC-20 and drain the contract after rule.

timestamps are unsafe for a few things

Timestamps should be replaced by block numbers as much as possible. For example, making the id of an item (itemSlot + block) to make it truly an unique id.
Also, timestamp period should be changed to an amount of blocks, and it shouldn't be immutable.

Remove all contribution logic

Contribution and appeal logic doesn't belong anymore, since V2 will take care of appeals internally.
This will remove a lot of code and a lot of tests

Refactor Account events

You could refactor some account events onto a single AccountUpdate that emits accountId and fullStake (either compressed or uncompressed, we'll figure it out)

• AccountFunded
• AccountWithdrawn

Sort of related, wallet as a name field is a very bad name. owner is better.

Use UpgradableProxy? Some thoughts on settings, overengineering and its consequences

Having an Upgradable Proxy is put into question.

Rationale

Stake Curate is complex. A vast effort has been made to cover all potential edge cases, but truth it, there are so many interactions that this may, as it is, be still very much vulnerable.
Also, most of the settings that dramatically increase the complexity of the code are related to stake and frontrunning attacks. Drain vulnerabilities are significantly more important that those, a frontrunning attack could take place and, if it were to be patched out, it wouldn't be a big cause of concern.

Consequences

Different gas costs

Gas costs would decrease, due to Proxy pattern being able to write settings directly onto the bytecode as constants. (2100 gas per cold setting slot + stack related would be saved)
Gas costs would increase due to Proxy pattern doing delegate calls to an implementation contract. (2600 gas once, if cold account. Depending on the implementation, 2100 gas for a cold read to check if sender is the proxy admin)

Precautionary measures that could be removed

Some preventive measures would not need to be featured into the contract until attacks related to them occur.

• allowing Arbitrator would not be necessary, unless some actor created an arbitrator in order to attack the contract.
  • alternatively, value burns could still be added, in which case it wouldn't even matter and Arbitrators would not require being whitelisted.
• most list legality measures won't need to be applied, unless:
  • minimum challenger stake ratios are ever increased.
  • maxAgeForInclusion is decreased
  • arbitrator allowance is introduced (but again, this requirement could be discarded with value burns)
  • minRetractionPeriod is increased

Some hardcodings of these settings could considered be final, thus defusing the need for list legality to even be checked. This is an example of a bunch of these settings that would hardly cause problems later if they could not be edited:

minChallengerStakeRatio

Having a lower minChallengerStakeRatio means burns need to be larger, and vice-versa. Likewise, lower timeout periods allow us to get away with lowering both of these. We would preferably have smaller sized burns. There's not much research on this topic, so all I can do is make assumptions. Let's assume we want attackers to have to burn, at the very least, 10% of the stake they're trying to frontrun every day:

• 4h for a commit timeout and 5% burns => 33.3% minChallengerStakeRatio
• 8h for a commit timeout, and 5% burns => 66.6% minChallengerStakeRatio
• 8h for a commit timeout, and 10% burns => 33.3% minChallengerStakeRatio

All in all I think 4h for commit timeout and 5% burns is probably fine. If commit timeout ever needs to increase, burns will need to raise, or list legality will need to be introduced.

maxAgeForInclusion

The reason this needs to be capped is a technical limitation. For items that have requiredAgeForInclusion > 0, they need to be checked through the balance records in order to check for inclusion. 4 balance records fit in one slot, so, ignoring the other overhead, that's:

1 cold SLOAD + 3 hot SLOAD per 4 balance records =>
2_100 + 3 * 100 = 2_400 =>
~600 gas per balance record access

Depending on the width of these balance records, these records could grow too large until going over the gas limit. At 30_000_000 gas, that's ~50k accesses. And then, we don't want to get too close to that because otherwise attackers could deliberately force others to spend a lot of gas reading item statuses. So, we can get down to Earth to something like 120 accesses tops, ~72_000 gas. With a balance record width of 6h, that's 40 days at a maximum, which should be beyond almost all intended use cases.

allowance arbitrator

As stated, this would likely not be needed if burning value is introduced.

minRetractionPeriod

1 day is fine. The only reason this was needed is to avoid attacks. This value should be orders of magnitude larger than minTimeForReveal, and ideally strictly larger than maxTimeForReveal, which would remove any possibility of attacks.

Other Settings

Since this became a general issue for refactoring, I'll mention other matters here.

• governor: no need, if proxy
• Remove smallBurnRate, have one single burn rate, and refund honest victims via governance. No need to mitigate it if full refunds were going to happen anyway
• withdrawalPeriod: 1 week
• challengeWindow: originally conceived to be 5 minutes. The larger it is, the less punishing it is to honest challengers that happen to commit a challenge right when the item is edited, or it takes too long for their commit to be included. But the larger it is, the more time challengers have to read changes between editions, and figure out if there was a past mistake that's being fixed, thus giving a chance to challenge the item. With commit reveal, this is mainly UX for these incidents, but not frontrun protection. So it could be lowered, or removed.
• burner: todo
• minTimeForReveal: the lower, better UX, less likely for naive users to forget revealing. the lower, higher chances for not enough gas, making their reveal not be mined in time, and someone able to commit and reveal fast enough. unlike something like ENS, where revealing a commit with a lower timestamp will net you ownership over the token, here the first to reveal wins. so, minTimeForReveal cannot be something like 1 min. 5 min may be safe enough. this value can be safely updated via proxy.
• withdrawalBurnRate: just, remove this. if someone builds the elaborate bot (tldr, a bot that handles withdrawalPeriod / maxTimeForReveal accounts that place well timed startWithdraw processes to be able to frontrun reveals with withdrawals in order to trigger full refunds), and a repeatable strategy is built around this scheme, then it can be added.
• timeForNextStake: maybe, this could just be equal to maxTimeForReveal, after all it's there just to stop users from frontrunning stake increases to make challenge reveals fail.