-
Notifications
You must be signed in to change notification settings - Fork 46
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
Esm ward test #334
base: master
Are you sure you want to change the base?
Esm ward test #334
Conversation
Co-authored-by: Arby <[email protected]>
This is draft PR. Added "WETH" and "USDC" to start. Will need to analyze this list and create/fix exceptions as necessary. https://gist.github.com/brianmcmichael/d756d321c3f68d6d97ad58f8deeb85e4 |
Test PASS at 456ef72: ➜ spells-mainnet git:(ESM_WARD_TEST) make test
./scripts/test-dssspell-forge.sh match="" block=""
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠰] Compiling...
[⠃] Compiling 7 files with 0.8.16
[⠰] Solc 0.8.16 finished in 3.39s
Compiler run successful
Running 2 tests for src/test/starknet.t.sol:StarknetTests
[PASS] testStarknet() (gas: 532119)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 1.07s
Running 18 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487102283060)
[PASS] testAuthInSources() (gas: 9223371487099283588)
[PASS] testBytecodeMatches() (gas: 1333016)
[PASS] testCastCost() (gas: 377556)
[PASS] testChainlogValues() (gas: 8625067)
[PASS] testChainlogVersionBump() (gas: 3759812)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 1321532)
[PASS] testESMWards() (gas: 9223371487100745242)
[PASS] testFailNotScheduled() (gas: 14375)
[PASS] testFailTooEarly() (gas: 13629)
[PASS] testFailTooLate() (gas: 13606)
[PASS] testFailWrongDay() (gas: 13607)
[PASS] testGeneral() (gas: 34575151)
[PASS] testNextCastTime() (gas: 353614)
[PASS] testOnTime() (gas: 373195)
[PASS] testPSMs() (gas: 1897685)
[PASS] testUseEta() (gas: 352346)
Test result: ok. 18 passed; 0 failed; finished in 60.39s |
"PROXY_DEPLOYER", // Ecosystem tool | ||
"WETH", // External contract, not PP ward | ||
"ETH", // External contract, not PP ward | ||
// Everything after this should be evaluated for security |
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.
A note for future maintainers of this codebase, that this list would need to be carefully inspected to ensure auth on ESM is not required (e.g. the mudule is self-protected during ES via vat.live
check or disabled by END.cage
or otherwise specified).
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.
Even if some module is protected, wouldn't it be better to always rely
on the ESM contract?
Other than additional gas costs or maybe a faulty ESM code (which shouldn't happen anyway), I can't see a good reason to not do it.
Otherwise, we have yet another blacklist to be manually maintained in the future.
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.
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.
I've read it before commenting. I believe you are referring to this part:
Generally, most modules could be excluded from needing to authorize the ESM based on simple criteria like not having wards, lacking sensitive permissions, having embedded safety mechanisms, being external to the system, or not being upgradable.
And:
Explicit justifications for “safe” contracts are not included here as these ought to be independently verified by whoever assumes responsibility for system safety post-PECU shutdown.
While I agree, I can't help but wonder if the cost of maintaining this blacklist of contracts that don't need to be checked would not be higher than simply having a rule to rely
on the ESM everywhere the PauseProxy
is rely
ed.
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.
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.
LGTM (pending resolving merge conflicts)
- Tests PASS at 890de84
./scripts/test-dssspell-forge.sh match="" block=""
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠒] Compiling...
[⠃] Compiling 105 files with 0.8.16
[⠘] Solc 0.8.16 finished in 5.72s
Compiler run successful
Running 2 tests for src/test/starknet.t.sol:StarknetTests
[PASS] testStarknet() (gas: 532119)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 8.33s
Running 18 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487102283060)
[PASS] testAuthInSources() (gas: 9223371487099283588)
[PASS] testBytecodeMatches() (gas: 1333016)
[PASS] testCastCost() (gas: 377556)
[PASS] testChainlogValues() (gas: 8625067)
[PASS] testChainlogVersionBump() (gas: 3759812)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 1321532)
[PASS] testESMWards() (gas: 9223371487100681717)
[PASS] testFailNotScheduled() (gas: 14375)
[PASS] testFailTooEarly() (gas: 13629)
[PASS] testFailTooLate() (gas: 13606)
[PASS] testFailWrongDay() (gas: 13607)
[PASS] testGeneral() (gas: 34575151)
[PASS] testNextCastTime() (gas: 353614)
[PASS] testOnTime() (gas: 373195)
[PASS] testPSMs() (gas: 1897685)
[PASS] testUseEta() (gas: 352346)
Test result: ok. 18 passed; 0 failed; finished in 538.50s
Adds a test of esm ward on every chainlog entry.
New additions to the chainlog that do not need to be a ward on the ESM will need to be added to the exceptions list.
For every exception, it would be helpful to include reason why.