Skip to content
Open
2 changes: 2 additions & 0 deletions .wordlist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ Repo
Solmate
SPDX
StairstepExponentialDecrease
StarGuard
Starknet
SubDAO
SubDAOs
Expand All @@ -90,6 +91,7 @@ authing
bytecode
casted
checksummed
codehash
collateralization
collaterals
config
Expand Down
7 changes: 7 additions & 0 deletions spell/spell-crafter-mainnet-workflow.md
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,13 @@ Repo: https://github.com/makerdao/spells-mainnet
* [ ] Changes are tested via `testNewOrUpdatedChainlogValues`
* [ ] Adjust system values, collateral values inside `config.sol`
* [ ] Ensure every spell variable is declared as public/internal
* IF Prime Agent spell is provided
* [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template
* [ ] IF `direct execution` is `no`
* [ ] The spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, update the name here as well.

* [ ] IF `direct execution` is `yes`
* [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` inside Core spell
* [ ] The spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));`
* Add specific tests in `DssSpell.t.sol` to have sufficient test coverage for every spell action
* [ ] Test new collaterals
* [ ] Test new ilk registry values
Expand Down
48 changes: 30 additions & 18 deletions spell/spell-reviewer-mainnet-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,27 +241,39 @@
* [ ] `MCD_VEST_MKR_TREASURY` chainlog address is used for MKR stream `yank`
* [ ] `MCD_VEST_DAI` chainlog address is used for DAI stream `yank`
* [ ] Tested via `testYankDAI` or `testYankMKR`
* IF SubDAO-related content is present
* IF SubDAO provides SubProxy spell address
* [ ] SubDAO spell address matches Exec Sheet
* [ ] Executed via `ProxyLike(SUBDAO_PROXY).exec(SUBDAO_SPELL, abi.encodeWithSignature("execute()"));`
* IF content related to a Prime Agent is present
* IF Prime spell is provided
* [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template
* [ ] IF `direct execution` is `no`
* [ ] The Prime spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)`
* [ ] `XXX` in `XXX_STARGUARD` matches the name of the Prime Agent
* [ ] `XXX_STARGUARD` is fetched from chainlog
* [ ] The test ensures the `XXX_SPELL` Prime spell is executable via `StarGuardLike(XXX_STARGUARD).exec()` before `XXX_STARGUARD.maxDelay`
* [ ] IF plotted but not yet executed spell is still present in the `XXX_STARGUARD`, Governance Facilitators are aware, already notified
* [ ] IF `direct execution` is `yes`
* [ ] Provided mandatory explanation of why direct execution is required makes sense on the technical level
* [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` inside the Core spell
* [ ] The Prime spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));`
* [ ] `XXX` in `XXX_PROXY` matches the name of the Prime Agent
* [ ] `XXX_PROXY` is fetched from chainlog
* [ ] Prime spell address (`XXX_SPELL`) matches Exec Sheet
* [ ] Prime spell hash (`XXX_SPELL_HASH`) matches Exec Sheet
* [ ] Execution is NOT delegate call
* [ ] IF SubDAO spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry
* [ ] Ensure that SubDAO spell have enough gas and does not revert with "out of gas" error inside simulation. Note: low level call gas estimation is not done by our scripts
* IF SubDAO provides instructions to be executed by the main spell (i.e. that will operate within Pause Proxy `DelegateCall` context)
* [ ] No SubDAO contract being interacted with is authed on a core contract like `vat`, etc. (Check comprehensively where the risk is high)
* [ ] SubDAO contract licensing and optimizations generally do not matter (except where they pose a security risk)
* [ ] SubDAO contracts and all libraries / dependencies have verified source code (Blocking)
* Upgradable SubDAO contracts
* [ ] IF Prime spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry
* IF Prime Agent provides instructions to be executed by the main spell (i.e. that will operate within Pause Proxy `DelegateCall` context)
* [ ] No Prime Agent contract being interacted with is authed on a core contract like `vat`, etc. (Check comprehensively where the risk is high)
* [ ] Prime Agent contract licensing and optimizations generally do not matter (except where they pose a security risk)
* [ ] Prime Agent contracts and all libraries / dependencies have verified source code (Blocking)
* Upgradable Prime Agent contracts
* [ ] Upgradable contracts have the `PAUSE_PROXY` as their `admin` (i.e. the party that can upgrade)
* [ ] Any upgradable SubDAO contracts with an `admin` that is not `PAUSE_PROXY` are not authed on any core contracts (Blocking)
* [ ] All SubDAO content addresses (i.e. provided contract addresses or EOAs) present in the Maker Core spell are present in the Exec Sheet and are correct. SubDAO addresses being authed or given any permissions MUST be in the Exec Sheet. SubDAO addresses being called must be confirmed by the SubDAO spell team.
* [ ] IF addresses not PR'ed in by the SubDAO team (use git blame for example), SubDAO content addresses all have inline comment for provenance or source being OKed by SubDAO
* [ ] SubDAO actions match Exec Sheet (only where inline with main spell code) and do not affect core contracts
* [ ] Any upgradable Prime Agent contracts with an `admin` that is not `PAUSE_PROXY` are not authed on any core contracts (Blocking)
* [ ] All Prime Agent content addresses (i.e. provided contract addresses or EOAs) present in the Maker Core spell are present in the Exec Sheet and are correct. Prime Agent addresses being authed or given any permissions MUST be in the Exec Sheet. Prime Agent addresses being called must be confirmed by the Prime Agent spell team.
* [ ] IF addresses not PR'ed in by the Prime Agent team (use git blame for example), Prime Agent content addresses all have inline comment for provenance or source being OKed by the Prime Agent
* [ ] Prime Agent actions match Exec Sheet (only where inline with main spell code) and do not affect core contracts
* [ ] Core contract knock-on actions (such as offboarding or setting DC to 0) are present in the exec and match the code
* [ ] External calls for SubDAO content are NOT delegate call
* [ ] Code does not have untoward behavior within the scope of Maker Core Contracts (e.g. up to the SubDAO proxy)
* IF external contracts calls are present (Not SubDAOs, e.g. Starknet)
* [ ] External calls for Prime Agent content are NOT delegate call
* [ ] Code does not have untoward behavior within the scope of Maker Core Contracts (e.g. up to the Prime Agent proxy)
* IF external contracts calls are present (Not Prime Agents, e.g. Starknet)
* [ ] Target Contract doesn't block spell execution
* [ ] External call is NOT `delegatecall`
* [ ] Target Contract doesn't have permissions on the Vat
Expand Down
16 changes: 15 additions & 1 deletion spell/star-spell-reviewer-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,15 @@ This section outlines the review process and provides concrete action items for
- [CHAIN_NAME] `0xADDRESS`, EXTERNAL_SOURCE_URL
- [ ] Matches valid external source (previously approved forum post, external docs, etc).

### StarGuard execution
- [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Star, the following additional checks are done:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Star, the following additional checks are done:
- [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Prime Agent, the following additional checks are done:

- [ ] The spell exposes view-only interface `function isExecutable() external view returns (bool result)`.
- [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g.: by describing "earliest launch date" or "office hours" logic, etc).
- [ ] The test is preset to ensure that `isExecutable` outputs `true` before `StarGuard.maxDelay()` expires the spell.
- [ ] Third-party actors can not take advantage of the fact that Spell will be executed in a later block than the Core spell, otherwise suggest `direct execution`.
- [ ] IF Prime spell can not be executed in a later block OR have to be executed sequentially to another Prime spell, `direct execution` is clearly proposed in the forum post together with elaborated explanation why it is needed.
- [ ] The `direct execution` explanation makes sense on the technical level and can not be circumvented by the use of `isExecutable()` interface.

#### On-boarding New Contracts
- LIST every new contract present in the spell:
- [CHAIN_NAME] `CONTRACT_NAME`, LINK_TO_THE_DEPLOYED_CONTRACT
Expand Down Expand Up @@ -240,6 +249,8 @@ EXECUTED_TESTS_LOGS
#### Deployed Contract
- [ ] Both reviewers gave explicit "Good to deploy".
- [ ] A new comment in the PR contains link to the deployed spell(s) and Tenderly vnet(s).
- [ ] The comment also contains codehash of the deployed mainnet spell.
- [ ] The codehash matches one produced locally from the reviewed source code.
- [ ] Every spell is verified on Etherscan or other primary block explorer for this chain.
- [ ] Every spell code matches local source code at the "good to deploy" commit.
- [ ] Etherscan settings (optimizer, EVM version, license) match local ones.
Expand All @@ -264,7 +275,10 @@ EXECUTED_TESTS_LOGS
#### Confirmed Handover
- [ ] Both reviewers gave explicit "Good to handover".
- [ ] All review comments have been addressed or resolved.
- [ ] The spell address posted by the crafter in the `#govops` thread matches the spell evaluated above.
- [ ] The spell address, the codehash and the direct execution are posted by the crafter in the `#govops` in the `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` format.
- [ ] Posted spell address matches the spell evaluated above.
- [ ] Posted spell codehash matches the one noted down above.
- [ ] Posted direct execution matches expected value.
- [ ] Confirm the address (via a separate "reply to" message, restating the address to avoid edits).
- [ ] Ensure that no changes were made to the code since the spell was deployed and archived.
- [ ] Approve spell PR for merge via 'Approve' review option.
Expand Down