Skip to content

Instantly share code, notes, and snippets.

@mschneider
Created Jan 12, 2022
Embed
What would you like to do?
FINAL
oki! like i said i dont have any showstopping attacks so heres my collected thoughts in no special order
* overall im really impressed with the design of your basic primitives
this is one of the first large non-anchor program ive worked on
and ngl i expected (hoped!) for low-hanging fruit
but that security checks flow through the program in a way thats very thoughtful and obviates most obvious issues
i spent a good amount of time trying to crack `get_account_data` to no avail
- no raw buffers are ever used
- ownership is always checked
- enums are always checked on deserialization and never modified
- seeds are explicitly enforced
- accounts are explicitly created in program
- the `get_x_for_y` pattern is very good for ensuring implicit ownership (namespacing?) is respected
- only leaf accounts in the implicit ownership chains are ever freed with `dispose_account`
these facts all conspired to thwart what i though would be a main line of attack
i even tried to do tricky shit by recursing through `process_execute_instruction` but couldnt accomplish anything
i was really impressed by how cleanly all this can be woven through a vanilla solana contract
im used to something more like the token-lending program, which just piles tons of checks into ever process function
if i have to write a vanilla solana program anytime i will def consult (steal from) governance
* i put a lot of attention into the interface boundary between the vote weight program and the main program
its very simple and there really isnt much attack surface here
you have a standard account format, and you check the account is owned by the realm-defined weight program
thats kinda it. if people fuck this up it will def be their own addin implementation
i would probably flesh out the example program a bit more to the point that its like
at least a drop-in replacement for the regular functionality. people will almost certainly mess it up
then again someone will probably just make a repo of generic addin programs sooner or later if theres demand
* the biggest concern i have about the whole thing is that addin can be enabled while realm authority is held by an eoa
sebastian said its "assumed realm authority is transferred" but this may be worth enforcing
eg, only allow setting a vote weight if realm authority is held by governance
and enforce upgrade authority on the addin program is also held by governance
otherwise the addin feature is a backdoor to bypass all governance entirely
this is a significant change from previous versions, where authority could only make incidental config changes
* in an ideal world it would not be possible to change the weight addin with any proposals in `ProposalState::Voting`
if the rules are changed, then you still have votes with the old rules which may well be higher than theyd be otherwise
i dont think theres a way to enforce this as designed. the realm cannot see into its goverances proposals
and to see their states would require passing accounts
the best i can think of is adding an increment/decrement when proposals move into/out of voting
but its probably not worth the added complexity. caveat suffragator
* the multiple choice implementation is simple and i didnt find much to poke at
if you end up allowing different weight percentages it might get dicey but with 100 or 0 its pretty foolproof
and the program properly checks that the absence of deny prevents instructions from being changed
i kinda hoped that proposals would be deallocated if cancelled so i could add instructions, delete, remake
but that isnt done, dashing my hopes on that front
* documentation is ok for where the project is. the flow chart is nice and much appreciated
that is, incomprehensible at first pass, but very simple and obvious once i read the code, and a useful reference
* lack of a simple js client was kind of annoying
i ended up ripping enough pieces out of oyster to be able to write my own test cases if needed
trying to use the rust test runner felt clumsy and this was actually the better option it seemed like
* create token governance should clear delegate and close authority on the account if ownership authority is transferred
LOG
46.5 hours total
11/15: 1300-1930 (6.5 hours)
wrote notes on state machine flow and all instructions
11/16: 1200-1530 (3.5 hours)
read process code
11/17: 1200-1800 (6 hours)
rust tests work, ctags work
planned my approach, read the addin-related code
11/18: 0900-1330 (4.5 hours)
investigation in how to structure my own js runner
(unexpected travel)
12/03: 1200-1630 (4.5 hours)
putting together my own test runner
12/04: 1400-2000 (6 hours)
first attempts using the runner
12/06: 1500-2200 (7 hours)
back to studying code
(sick)
12/14: 1200-1700 (5 hours)
failed to spoof accounts, posited some other attacks
12/19: 1200-1530 (3.5 hours)
final pass on code, found nothing really exciting
@acamill
Copy link

acamill commented Jan 30, 2022

👏

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