-
Notifications
You must be signed in to change notification settings - Fork 434
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
Auction bidder sanction check #876
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for nouns-home ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for nouns-testnet ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks clean implementation wise. Some thoughts on the related risks and design implications:
-
sanctionsOracle
becomes an another point of trust: it can turn rogue, block all other bids by marking the bidders sanctioned (say just when theisSanctioned()
request comes from the Auction House, behaving normally otherwise) and keep winning the auctions for non-material amounts until auctions become paused andsanctionsOracle
gets replaced -
There is also a possibility of collision by an attacker and
sanctionsOracle
controlling party, which can selectively revert bids when the attacker pursues the auctioned noun, splitting the difference between realized and current market prices -
If there be a known algorithm (or even a heuristics being correct frequently enough) for marking an address sanctioned, say a tx from an already sanctioned address with a value above some threshold makes its recipient sanctioned too, then it can be exploited by any third party that controls the sanctioned address, i.e. they can bid small on each auction, then front run all the other bids with such tx from the sanctioned address they control, reverting all the other bids and getting the noun at a depressed price
-
In order to control for operational mistakes it can be recommended to check Oracle availability, e.g.
require(!sanctionsOracle_.isSanctioned(address(this)), 'Sanctions Oracle malfunction')
on setting the new Oracle ininitialize()
andsetSanctionsOracle()
-
sanctionsOracle
non-malicious malfunction will freeze auctions:createBid()
cannot proceed whensanctionsOracle_.isSanctioned(account)
call reverts. If the sale without sanctions check is feasible consider wrapping the call in a try-catch block and place Oracle unavailability case logic in the catch section. This paired with (6) can suggest some auction time extension logic (i.e. Oracle was checked at the start, as (6) suggests, then it became unavailable, so the auction was unavailable too for some time in the middle and should be extended to a degree) -
In order to reduce the number of situations when Oracle was unavailable for a part of the particular auction timeline, so the amount of bids and the resulting price was lower due to that only, consider checking the Oracle availability in
_createAuction()
(reverting if Oracle isn't available) and_settleAuction()
(if (5) won't be implemented, socreateBid()
fails silently and don't affect the auction, there is a lighter alternative: not settling, but extending the auction if Oracle isn't available on_settleAuction()
)
No description provided.