chore: improve star reviewer checklist#60
chore: improve star reviewer checklist#60SidestreamStrongStrawberry wants to merge 3 commits intomasterfrom
Conversation
| - [ ] IF source code is not audited, there is a clear explanation that was agreed upon by governance beforehand (i.e.: reusing unaudited contracts with lots of Lindy effect). | ||
| - [ ] Compilation optimizations match deployment settings defined in the source code repo. | ||
| - [ ] Consistent license. | ||
| - [ ] Deployer address was not used on other chains UNLESS there is a valid reason for it (e.g., external contract, the same deployer was used to keep addresses the same across chains, etc). |
There was a problem hiding this comment.
Other chains is too vague imo. Reviewers can't check all possible EVM chains. Should specify which chains the reviewers need to check.
| - [ ] Expected admin address for this chain has full access (`SubProxy` on mainnet, `Executor` on other chains). | ||
| - [ ] Contract deployer address has no access (e.g. `wards(deployer)` is `0`). | ||
| - [ ] No other addresses has access to this contract. | ||
| - [ ] IF the contract is a vault, each vault role that can have access to the funds has to be validated against trusted external sources (i.e. docs listing contracts which have that role in the vault) or against other verifiable sources. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Vault is vague from a technical viewpoint.
I think this is rather more on functional side so what the smart contract do.
Main point here is verifying the access to big amount of tokens.
As a reviewer what is a trusted external source or other verifiable sources?
IMO, it can be previously onboarded contract, or any form of off-chain information from the vault owner.
I agree this sounds vague. I wanted to make it bit more general instead the action locked to 1 possible solution which hopefully allows reviewers to make the correct judge on what makes sense at the point of review.
As we are using the checklist, if we fix a custom for checking this value we can always update.
But also if you prefer a specific check, feel free to suggest!
This seems like it might make more sense as part of the wards sub-checklist?
Wards check already have IF branch, which means the sub checks are not done when the contract doesn't have wards (or auth) concept.
So I'd prefer keeping them separately.
| - [ ] Specify the correct spell target data: | ||
| ``` | ||
| YYYY-MM-DD | ||
| ``` |
There was a problem hiding this comment.
Formatting nit, This takes up a lot of space and I'm not sure having the reviewer write out the date warrants that... maybe inline?
| - [ ] IF source code is not audited, there is a clear explanation that was agreed upon by governance beforehand (i.e.: reusing unaudited contracts with lots of Lindy effect). | ||
| - [ ] Compilation optimizations match deployment settings defined in the source code repo. | ||
| - [ ] Consistent license. | ||
| - [ ] Deployer address was not used on other chains that star is onboarded UNLESS there is a valid reason for it (e.g., external contract, the same deployer was used to keep addresses the same across chains, etc). |
There was a problem hiding this comment.
What's the context on this? Why is it a problem?
Reading from a fresh perspective, seems like an item that won't add anything beside more work to reviewers.
There was a problem hiding this comment.
This was added to avoid the contract addresses collision on different chains. As it can be confusing when the different contracts on the different chains use the same address.
There was a problem hiding this comment.
On a lot of cases, address collision on different chains is a feature, not a bug. And even if non-intentional, I'm not sure if it's a problem at all.
| - [ ] Expected admin address for this chain has full access (`SubProxy` on mainnet, `Executor` on other chains). | ||
| - [ ] Contract deployer address has no access (e.g. `wards(deployer)` is `0`). | ||
| - [ ] No other addresses has access to this contract. | ||
| - [ ] IF the contract is a vault, each vault role that can have access to the funds has to be validated against trusted external sources (i.e. docs listing contracts which have that role in the vault) or against other verifiable sources. |
There was a problem hiding this comment.
Does the whole section refers mostly to "internal" (start deployed) or "external" (other protocols) contracts?
IF it's external, then I don't think this item is reasonable, as it would require the reviewer in depth understanding of another protocol access control rules and internal key management policies.
IF it's internal, then this might be fine, although I don't think it brings that much extra security.
ps. I do think this checklist would benefit from a clear separation between internal onboarded and external onboarded contract. The section was inspired by core checklist, which rarely interact with external protocols, but the scenario is the exact opposite for starts.
There was a problem hiding this comment.
The Vault contract can be external or internal.
I think this can make the trust level stronger, as it does not only require to provide the address who has access to funds but also requires the source of the address.
For example, if an external vault contract is onboarded, the address who can access to contract owned tokens should be verified by a reliable source and this can add extra layer of transparency.
Probably star team already has done this in their process, so it would be mainly the reviewer who needs to do extra validation.
This approach still has limitations but I hope this can make the system more trustworthy!
Let me know if you have any other suggestions on this! :)
There was a problem hiding this comment.
I personally don't think this is reasonable for a start checklist, specially given how short the time for reviewing this stuff is. Vaults comes in many different flavors and each protocol has its own process to on how to manage access control and permissions.
Besides, from a first principle, this should be a risk/due-dilligence review, not a technical one. I don't think it's wise to take this into our own hands as our job is to just make sure that whatever the stars decides to execute is technically viable and won't break downstream and that's it. If we start having opinions on how good an investment is based on loose concepts like addresses that can access the funds, we become liable for future investments.
That's my opinion tho: including an item like this is we would be shooting ourselves in the foot on a massive way, for a very marginal "trust" benefit.
3 participants
This PR improves start reviewer checklist with points collected from 2026-01-15 spell