dao-report.txt

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

👏