makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
107 stars 43 forks source link

2023-06-28 #353

Closed 0xdecr1pto closed 1 year ago

0xdecr1pto commented 1 year ago

Description

Contribution Checklist

Checklist

DaiFoundation-DevOps commented 1 year ago

CLA assistant check
All committers have signed the CLA.

SidestreamSweatyPumpkin commented 1 year ago

❗ ~Pending exec hash.~

Checklist



./scripts/test-dssspell-forge.sh match="" block=""
load: 1.43  cmd: cast 14561 waiting 0.10u 0.02s
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠃] Compiling...^[
No files changed, compilation skipped

Running 2 tests for src/test/starknet.t.sol:StarknetTests
[PASS] testStarknet() (gas: 1388548)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 36.97s

Running 25 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487104948972)
[PASS] testAuthInSources() (gas: 9223371487099409252)
[PASS] testBytecodeMatches() (gas: 3071526)
[PASS] testCastCost() (gas: 1236463)
[PASS] testChainLinkPaymentAdapterTreasury() (gas: 1245892)
[PASS] testChainlogValues() (gas: 9874291)
[PASS] testChainlogVersionBump() (gas: 3800382)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 3055254)
[PASS] testFailNotScheduled() (gas: 14361)
[PASS] testFailTooEarly() (gas: 417149)
[PASS] testFailTooLate() (gas: 417428)
[PASS] testFailWrongDay() (gas: 417023)
[PASS] testGeneral() (gas: 37530133)
[PASS] testMKRPayments() (gas: 1377475)
[PASS] testNewChainlogValues() (gas: 1255189)
[PASS] testNextCastTime() (gas: 446365)
[PASS] testOnTime() (gas: 1232147)
[PASS] testPSMs() (gas: 2578711)
[PASS] testPayments() (gas: 1302743)
[PASS] testRWA007DocChange() (gas: 1312682)
[PASS] testRWA015_INTEGRATION_CONDUITS_SETUP() (gas: 1263524)
[PASS] testRWA015_OUTPUT_CONDUIT_DEPLOYMENT_SETUP() (gas: 25358)
[PASS] testUseEta() (gas: 352492)
[PASS] test_RWA015_ORACLE_PRICE_BUMP() (gas: 1259978)
Test result: ok. 25 passed; 0 failed; finished in 952.17s
The-Arbiter commented 1 year ago

Hi @0xdecr1pto , I am not a part of the spell team for this one and so am not a reviewer but have been requested to keep an eye on things to ensure that this goes out correctly. I've noticed a few things which you might want to address.

Please note that I am not a part of the spell team nor am I a reviewer for this spell so please treat this as externally provided feedback which does not need to be incorporated or have changes made if the spell team does not decide to make the changes, as spellcrafting decisions are only made by the three members of the spell team for a given spell (of which I am not part for this week).

That being said, please see below for some issues I spotted with this spell.

Major

Minor

Style

Nits

These changes can be incorporated by the spell team if they (independently) decide that these are valid concerns. Any and all elements of this feedback can be ignored and/or overridden by the current spell team (Decr1pto, SidestreamSweatyPumpkin and Amusingaxl) if they wish as ultimately these are the only parties who can make spell-related decisions.

0xdecr1pto commented 1 year ago

MINOR - RWA015_A_OUTPUT_CONDUIT.hope(address(this)) is missing (i.e. not hoping Pause Proxy. This was done for RWA015 onboarding) MINOR - RWA015_A_OUTPUT_CONDUIT.mate(address(this)) is missing (i.e. not mating Pause Proxy. This was done for RWA015 onboarding)

@The-Arbiter This two wasn't done in RWA015 onboarding (see archive)

SidestreamSweatyPumpkin commented 1 year ago

@0xdecr1pto (https://github.com/makerdao/spells-mainnet/pull/353#issuecomment-1606119036) i think @The-Arbiter has a point. The following lines from the bootstrap segment are doing the mentioned actions.

https://github.com/makerdao/spells-mainnet/blob/2a79dc3347cd283d43372c599562c0416710d3e0/archive/2023-06-14-DssSpell/DssSpell.sol#L251-L252

On top of this, if i recall correctly - the bootstrap part was done for rwa015 in the spell (this is different from the previous rwa-s). So the corresponding approach should be used here. So it seems logical to either

  1. also have the bootstrap-like part (link above) and have the corresponding (to rwa15 onboarding) test setup (some tests altered/disabled compared to e.g. 014)
  2. Or have no bootstrap part, and have the similar to 014 test setup (unaltered/all enabled)

As of per-point opinion about https://github.com/makerdao/spells-mainnet/pull/353#issuecomment-1606105511 :

Major

  • MAJOR - RWA015_A_OUTPUT_CONDUIT ChainLog update works but it doesn't scuttle the old one --> It remains a core contract and can be bug bountied. Is that one (USDC) intended to be used in the future? If so, this needs to be kept in our addresses_mainnet.sol file and kept in the chainlog. Bug bounties can be claimed against this contract and it still has the same permissions (it is not being scuttled). Setup also does not match previous pattern for RWA014 or RWA015, so this needs fixing.
  • MAJOR - Test coverage for the new output conduit is abysmal testRWAXXX_INTEGRATION_CONDUITS_SETUP is wrong (IMO) testRWAXXX_SPELL_LOCK_OPERATOR_DRAW_WIPE_FREE is not present testFailRWAXXX_DRAW_ABOVE_LINE is not present testFailRWAXXX_OUTPUT_CONDUIT_PUSH_ABOVE_BALANCE is not present testRWAXXX_OPERATOR_LOCK_DRAW_CAGE is not present This would be a blocker under PE.

Should be addressed: have all the mentioned tests. If one should be disabled, provide the reason in the comment. The test content should be determined by the previous archives and by how the spell contents relate to the rwa015 onboarding (bootstrap-like part present or not)

Minor

  • MINOR - RWA015_A_OUTPUT_CONDUIT.hope(address(this)) is missing (i.e. not hoping Pause Proxy. This was done for RWA015 onboarding)
  • MINOR - RWA015_A_OUTPUT_CONDUIT.mate(address(this)) is missing (i.e. not mating Pause Proxy. This was done for RWA015 onboarding)
  • MINOR - Dropping RWA015_A_OUTPUT_CONDUIT from addresses_mainnet.sol rather than naming it _LEGACY which is the pattern for deprecated items.

All minors above should be addressed. The first two: either provide reason to not add these lines or adding these lines. The last one: should be addressed.

Style

  • STYLE - Delegate compensation payment ordering doesn't match sheet (6 of them are different, this just makes reviewing more error-prone).

Ideally yes

  • STYLE - PSM-GUSD-A autoline change should be a call to setIlkAutoLineDebtCeiling as only line is being changed (this reduces the surface area for mistakes / human error).

Ideally yes

  • STYLE - Office Hours on but function is not deleted, which is precedent

I personally don't understand this point. To get what's meant i need elaboration/link to an example.

  • STYLE - Commit hash doesn't match (content OK) but doesn't match precedent pattern

Ideally yes.

Nits

All make sense, unless specified i'm not going to demand/block progress if not addressed. = if you have time: please adjust.

0xdecr1pto commented 1 year ago

@SidestreamSweatyPumpkin "@0xdecr1pto (https://github.com/makerdao/spells-mainnet/pull/353#issuecomment-1606119036) i think @The-Arbiter has a point. The following lines from the bootstrap segment are doing the mentioned actions."

Check the next few lines and you will find that all this permission was revoked and they was needed only for bootstrapping

0xdecr1pto commented 1 year ago

Regarding the tests: this is an onboarding test and in our case, we just changing one component of this structure. So it doesn't make a lot of sense to test all of the components.

I will add a few of them but not all. Let me know if you need more details explaining the structure of RWA deals

SidestreamSweatyPumpkin commented 1 year ago

Check the next few lines and you will find that all this permission was revoked and they was needed only for bootstrapping

Alright. I've looked again at the exec sheet for 2023-06-07, the forum post for the current rwa015 changes and the current exec sheet. draw is not requested in this spell , so the lines there are not required by exec sheet. However, it then means that .+_DRAW_.+ tests are needed to ensure that it all works together.

0xdecr1pto commented 1 year ago

Check the next few lines and you will find that all this permission was revoked and they was needed only for bootstrapping

Alright. I've looked again at the exec sheet for 2023-06-07, the forum post for the current rwa015 changes and the current exec sheet. draw is not requested in this spell , so the lines there are not required by exec sheet. However, it then means that .+_DRAW_.+ tests are needed to ensure that it all works together.

We need a test for integration urn with conduit, something like Urn.draw -> conduit.push this will be enough to test new conduit

SidestreamSweatyPumpkin commented 1 year ago

Tests for latest version:

[PASS] testStarknet() (gas: 1474807)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 26.16s

Running 28 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487105110254)
[PASS] testAuthInSources() (gas: 9223371487099402769)
[PASS] testBytecodeMatches() (gas: 3184984)
[PASS] testCastCost() (gas: 1322722)
[PASS] testChainLinkPaymentAdapterTreasury() (gas: 1332217)
[PASS] testChainlogValues() (gas: 9976173)
[PASS] testChainlogVersionBump() (gas: 3884619)
[PASS] testContractSize() (gas: 8962)
[PASS] testDeployCost() (gas: 3168406)
[PASS] testFailNotScheduled() (gas: 14361)
[PASS] testFailRWA015_A_OUTPUT_CONDUIT_PUSH_ABOVE_BALANCE() (gas: 1469586)
[PASS] testFailTooEarly() (gas: 417127)
[PASS] testFailTooLate() (gas: 417549)
[PASS] testFailWrongDay() (gas: 417617)
[PASS] testGeneral() (gas: 37729293)
[PASS] testMKRPayments() (gas: 1463801)
[PASS] testNewChainlogValues() (gas: 1356926)
[PASS] testNextCastTime() (gas: 446343)
[PASS] testOnTime() (gas: 1318384)
[PASS] testPSMs() (gas: 2665037)
[PASS] testPayments() (gas: 1388923)
[PASS] testRWA007DocChange() (gas: 1398919)
[PASS] testRWA015_INTEGRATION_CONDUITS_SETUP() (gas: 1349783)
[PASS] testRWA015_OPERATOR_DRAW_CONDUIT_PUSH() (gas: 1726998)
[PASS] testRWA015_OUTPUT_CONDUIT_DEPLOYMENT_SETUP() (gas: 25423)
[PASS] testRWA015_REVOKE_OPERATOR_ON_LEGACY_OUTPUT_CONDUIT() (gas: 1326830)
[PASS] testUseEta() (gas: 352492)
[PASS] test_RWA015_ORACLE_PRICE_BUMP() (gas: 1346237)
Test result: ok. 28 passed; 0 failed; finished in 953.68s
amusingaxl commented 1 year ago

Historically, I get why those RWA spell tests were there, as MIP21 was a proof of concept and not thoroughly tested and audited.

MAJOR - RWA015_A_OUTPUT_CONDUIT ChainLog update works but it doesn't scuttle the old one --> It remains a core contract and can be bug bountied.

We (Dewiz) have agreed in conversations with the former PE team that the only RWA component that is to be considered a core component is the RwaUrn and it's derivatives (i.e.: RwaUrn2), because they are authorized on the Vat.

Specially when it comes to conduits, they should be treated as 3rd-party code. Conduits won't fall into a bug hunter's radar because:

  1. They have no permissions within the protocol.
  2. They only interact with assets the contract itself owns or with permissionless functions on other components (i.e.: swap conduits)
  3. They can only send assets to addresses that have been authorized by Maker governance previously, which means that the extent of damage that can be done by an attacker negligible.
  4. Any issues caused by an attacker can be reverted by governance.

Is that one (USDC) intended to be used in the future? If so, this needs to be kept in our addresses_mainnet.sol file and kept in the chainlog.

Regarding the chainlog, it's subjective that it must be there, because we never had a case of a replaced conduit before. Last time we discussed that with PE we were even considering removing all RWA contracts, except for the RWW Urns and RWA Tokens from the Chainlog. Still, it's not a bad idea to keep it there.

Setup also does not match previous pattern for RWA014 or RWA015, so this needs fixing.

Please notice that this is not an RWA onboarding, it's simply a component being replaced. Also you can see from the archive that every RWA deal has slightly different characteristics, so the same pattern will not be followed every single time.

Bug bounties can be claimed against this contract and it still has the same permissions (it is not being scuttled)

Explained above why bug bounties are not a problem for conduits. Regarding the permissions:

  1. This contract might have to be used in the future, that's why we can't de-auth the pause proxy right now.
  2. operator and mate access can still be active, but innefective in practice, as the old conduit is not the target of the RWA Urn anymore. No MakerDAO assets will be sent to this contract after the spell is crafted.
  3. We are implementing a new type of conduit that will supersede both the current one and the one being removed. At that point, the full de-authing can be made in both of them simultaneously.
  • MAJOR - Test coverage for the new output conduit is abysmal testRWAXXX_INTEGRATION_CONDUITS_SETUP is wrong (IMO) testRWAXXX_SPELL_LOCK_OPERATOR_DRAW_WIPE_FREE is not present testFailRWAXXX_DRAW_ABOVE_LINE is not present testFailRWAXXX_OUTPUT_CONDUIT_PUSH_ABOVE_BALANCE is not present testRWAXXX_OPERATOR_LOCK_DRAW_CAGE is not present This would be a blocker under PE.

This is a clear case where "oh, it has always been done like that" is not a strong argument. Dewiz today has more domain knowledge regarding RWAs than the people who initially wrote the first set of RWA components. In fact, not only we understand back-to-back every aspect of the original components, but we also were the ones who wrote all the new ones.

Let's take a look at those:

testRWAXXX_INTEGRATION_CONDUITS_SETUP is wrong (IMO)

The only thing that was changed in this spell was the output conduit. This test is a sanity check to see if the right permissions were in place after the spell. It is correct.

testRWAXXX_SPELL_LOCK_OPERATOR_DRAW_WIPE_FREE is not present

The objective of this test if check whether debt can be wiped and the RWA Token can be freed afterward. It does touch the output conduit, but the flow could start later, and the coverage would remain the same. Only the input conduit and the urn are relevant here. Therefore, this is not needed, because this test was executed when we onboarded RWA015.

testFailRWAXXX_DRAW_ABOVE_LINE is not present

This test touches only the urn, it was already executed when we onboarded RWA015.

testFailRWAXXX_OUTPUT_CONDUIT_PUSH_ABOVE_BALANCE is not present

This is actually more of a unit test for RwaSwapOutputConduit than anything else. It's already covered in the rwa-toolkit repo. This should not be in the spell test suite.

testRWAXXX_OPERATOR_LOCK_DRAW_CAGE is not present

Emergency shutdown is not affected by a different output conduit. Only the outstanding debt for the urn is relevant here.

SidestreamSweatyPumpkin commented 1 year ago
./scripts/test-dssspell-forge.sh match="" block=""
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠢] Compiling...
[⠢] Compiling 104 files with 0.8.16
[⠔] Solc 0.8.16 finished in 4.08s
Compiler run successful

Running 2 tests for src/test/starknet.t.sol:StarknetTests
[PASS] testStarknet() (gas: 1474751)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 37.78s

Running 27 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487105110254)
[PASS] testAuthInSources() (gas: 9223371487099402769)
[PASS] testBytecodeMatches() (gas: 3184984)
[PASS] testCastCost() (gas: 1322666)
[PASS] testChainLinkPaymentAdapterTreasury() (gas: 1332161)
[PASS] testChainlogValues() (gas: 9976117)
[PASS] testChainlogVersionBump() (gas: 3884563)
[PASS] testContractSize() (gas: 8962)
[PASS] testDeployCost() (gas: 3168406)
[PASS] testFailNotScheduled() (gas: 14361)
[PASS] testFailTooEarly() (gas: 417614)
[PASS] testFailTooLate() (gas: 417549)
[PASS] testFailWrongDay() (gas: 417617)
[PASS] testGeneral() (gas: 37731334)
[PASS] testMKRPayments() (gas: 1463745)
[PASS] testNewChainlogValues() (gas: 1356870)
[PASS] testNextCastTime() (gas: 446343)
[PASS] testOnTime() (gas: 1318328)
[PASS] testPSMs() (gas: 2664981)
[PASS] testPayments() (gas: 1388397)
[PASS] testRWA007DocChange() (gas: 1398863)
[PASS] testRWA015_INTEGRATION_CONDUITS_SETUP() (gas: 1349727)
[PASS] testRWA015_OPERATOR_DRAW_CONDUIT_PUSH() (gas: 1727163)
[PASS] testRWA015_OUTPUT_CONDUIT_DEPLOYMENT_SETUP() (gas: 25423)
[PASS] testRWA015_REVOKE_OPERATOR_ON_LEGACY_OUTPUT_CONDUIT() (gas: 1326774)
[PASS] testUseEta() (gas: 352492)
[PASS] test_RWA015_ORACLE_PRICE_BUMP() (gas: 1346181)
Test result: ok. 27 passed; 0 failed; finished in 958.27s