2023-04-28 - Spark D3M + Configuration Changes #339

hexonaut · 2023-04-26T08:38:03Z

Description

Contribution Checklist

PR title starts with (PE-<TICKET_NUMBER>)
Code approved
Tests approved
CI Tests pass

Checklist

amusingaxl

Checklist

Office Hours
- On (Collateral Onboarding, Keepers, Integrations, ...)
- matches exec doc
Exec Hash
- Run make exec-hash for current date, or date=YYYY-MM-DD
  - Executive vote file name and date is correct
  - community repo commit hash corresponds to latest change
  - Raw GitHub URL is correct
  - Exec hash is correct
- description date in DssSpell.sol matches exec copy date
30 Days Expiry
lib
- dss-exec-lib
  Running git submodule status lib/dss-exec-lib:
```
69b658f35d8618272cd139dfc18c5713caf6b96b lib/dss-exec-lib (v0.0.9-7-g69b658f)`
```
  ✅ Matches 69b658f3
  - if submodule upgrades are present make sure dss-exec-lib is synced as well
  - git submodule hash matches tag latest release version or above
- dss-test
  - dss-interfaces
    - git submodule hash matches github master commit
      Running cd lib/dss-test/; git submodule status lib/dss-interfaces; cd -:
      9bfd7afadd1f8c217ef05850b2555691786286cb lib/dss-interfaces (heads/master)
      ✅ Matches 9bfd7afa
  - forge-std
    - git submodule hash matches github tag latest release version
      Running cd lib/dss-test/; git submodule status lib/forge-std; cd -:
      aea0b2685bebc883c09f5554d7fb481e85d0564d lib/forge-std (v1.2.0-9-gaea0b26)
      ❗ 30 commits behind b971f66
dss-interfaces
- used in the current spell
- cleanup previous ones
- ensure only single import layout is used (e.g. import "dss-interfaces/dss/VatAbstract.sol";)
Static Interfaces
- ensure they match dss-interfaces
- check on-chain interface of deployed contract via cast interfaces <contract_address> to ensure correctness
- interface naming style should match with Like suffix (e.g. VatLike)
  ❗ ID3MPool should've been named D3MPoolLike
- ensure they only list used functions in spell code
  ❗ There are some dangling interfaces and structs
Math matches
- Internal Precision
  - WAD = 10**18
  - RAY = 10**27
  - RAD = 10**45
  - Ensure they match with ds-math and the Numerical Ranges
  - Variable visibility declared as internal
- Units
  - MILLION = 10**6
  - Ensure they match with config
  - Variable visibility declared as internal

Deployed Contracts

Verified on etherscan
Optimizations match Repo
❓ dss-direct-deposit/foundry.toml does not explicitly set optimizations config, so it sticks with Foundry's default which is solc_optimizer = true; optimizer_runs = 200;.
GNU AGPLv3 license
Constructor args ok (e.g. vat, dai, dog, ...)
- Match ChainLog
  ✅ Did not check manually, but there are sanity checks for all of the relevant parameters and those are good to go.

Wards ok (pause proxy relied, deployer denied) ✅ ❕ (there are a couple of upgradeable contracts that needed to be checked):
I used this test suite:

// SPDX-License-Identifier: AGPL-3.0-or-later
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

interface ProxyWithAdminLike {
    function admin() external returns (address);
}

interface Ownable {
    function owner() external view returns (address);
}

contract ProxyOwnerCheckerTest is Test {
    // MakerDAO Governance Pause Proxy
    address constant MCD_PAUSE_PROXY   = 0xBE8E3e3618f7474F8cB1d074A26afFef007E98FB;
    // Upgradeable Proxy to a PoolConfigurator instance
    address constant PROXY_ADMIN       = 0x542DBa469bdE58FAeE189ffB60C6b49CE60E0738;
    // PoolAddressesProvider
    address constant PROXY_ADMIN_ADMIN = 0x02C3eA4e34C0cBd694D2adFa2c690EECbC1793eE;

    // This contract is owned by the PoolAddressesProvider directly
    address POOL_CONFIGURATOR = 0x542DBa469bdE58FAeE189ffB60C6b49CE60E0738;

    // These contracts are owned by nested upgradeable proxies
    address[3] proxyContracts = [
        /* SPARK_ADAI = */              0x4DEDf26112B3Ec8eC46e7E31EA5e123490B05B8B,
        /* SPARK_DAI_STABLE_DEBT = */   0xfe2B7a7F4cC0Fb76f7Fc1C6518D586F1e4559176,
        /* SPARK_DAI_VARIABLE_DEBT = */ 0xf705d2B7e92B3F38e6ae7afaDAA2fEE110fE5914
    ];

    function testProxyAdmin() public {
        vm.prank(PROXY_ADMIN_ADMIN);
        address poolConfiguratorAdmin = ProxyWithAdminLike(POOL_CONFIGURATOR).admin();
        assertEq(poolConfiguratorAdmin, PROXY_ADMIN_ADMIN);

        for (uint256 i = 0; i < proxyContracts.length; i++) {
            // Retrieve the admin to the contracts themselves...
            vm.prank(PROXY_ADMIN);
            address proxyAdmin = ProxyWithAdminLike(proxyContracts[i]).admin();
            assertEq(proxyAdmin, PROXY_ADMIN);

            // Retrieve the admin of the admin of the contract...
            vm.prank(PROXY_ADMIN_ADMIN);
            address proxyAdminAdmin = ProxyWithAdminLike(proxyAdmin).admin();
            assertEq(proxyAdminAdmin, PROXY_ADMIN_ADMIN);

            address ultimateOwner = Ownable(proxyAdminAdmin).owner();
            assertEq(ultimateOwner, MCD_PAUSE_PROXY);
        }
    }
}

Running forge test -vvv --fork-url=<MAINNET_RPC_URL> results in:

Running 1 test for test/MainnetProxyOwnerChecker.t.sol:ProxyOwnerCheckerTest
[PASS] testProxyAdmin() (gas: 34215)
Test result: ok. 1 passed; 0 failed; finished in 2.29s

Check whether the contract requires to rely the ESM in the spell (in order to allow de-authing the pause proxy during Emergency Shutdown, via denyProxy).

Matches corresponding github source code (i.e. diffcheck via vscode code --diff etherscan.sol github.sol)
Ensure deployer address is included into addresses_deployers.sol (to keep up to date)

Onboarding
- Collateral Onboarding
ChainLog
- Bump ChainLog, accordingly with spec (major, minor, patch)
  - PATCH -> Collateral addition or addition/modification
addresses_mainnet.sol matches spell code
Ensure every spell variable is declared as public/internal
Ensure immutable visibility is only used when fetching addresses from the ChainLog via DssExecLib.getChangelogAddress and constant is used instead for static addresses
Spell actions match GovAlpha Spell Content Sheet and hashed exec doc
Tests PASS
- Ensure Good Coverage
- Ensure every test function is declared as public if enabled or private if disabled
Local Tests and CI PASS

src/DssSpell.sol

src/DssSpell.t.sol

src/test/addresses_mainnet.sol

rockyfour

LGTM to deploy.

Chain Security deployment report

I went through the reports and the findings seems to be alright, excepting for those cases where we know there are special contracts, SparkLend implements the same contracts than Aave deployment or the current Aave repo version.

From all the CS claims, every contract that corresponds seems to be owned by Pause Proxy.

Regarding the configuration set up, there are some discrepancy on the risk parameters. We should assume this is under Risk responsability to evaluate each of them.

Most importantly I did not find anything in the report that raises a warning from CS on regard to the deployment.

Chain Security DAI Strategy and sDAI oracle

All seems good and bugs have been not found by the audit.

Mainnet Spell

D3M deployment dependencies match code in D3M repo master branch
SPARK_D3M_PLAN (0x104FaDbb7e17db1A685bBa61007DfB015206a4D2) code matches repo, deployed with correct version of solc and optimization flags. Constructor params are correct. Pause proxy relied, deployer denied.
SPARK_D3M_POOL (0xAfA2DD8a0594B2B24B59de405Da9338C4Ce23437) code matches repo, deployed with correct version of solc and optimization flags. Constructor params are correct (assuming Spark pool is correct). Pause proxy relied, deployer denied.
SPARK_D3M_ORACLE (0xCBD53B683722F82Dc82EBa7916065532980d4833) code matches repo, deployed with correct version of solc and optimization flags. Constructor params are correct. Pause proxy relied, deployer denied.
SPARK_ADAI (0x4DEDf26112B3Ec8eC46e7E31EA5e123490B05B8B) - matches audited address.
SPARK_DAI_STABLE_DEBT (0xfe2B7a7F4cC0Fb76f7Fc1C6518D586F1e4559176) - matches audited address.
SPARK_DAI_VARIABLE_DEBT (0xf705d2B7e92B3F38e6ae7afaDAA2fEE110fE5914) - matches audited address.
-SPARK_POOL_CONFIGURATOR (0x542DBa469bdE58FAeE189ffB60C6b49CE60E0738) - matches audited address.
-INTEREST_RATE_STRATEGY (0x113dc45c524404F91DcbbAbB103506bABC8Df0FE) - matches audited address.
D3MPlan/Pool/Oracle for Spark use correctly the previously reviewed dependency of the dss-direct-deposit repo to get set up.
Risk parameters for the D3M match the doc (maxLine, gap, ttl, tau and buffer)
Risk parameters changes of Spark seem to match the doc. Basically they are sounded but we are not digging if the calls are supposed to be doing what they claim, nor if the paremeters are correctly set either.
Tests look good
Tests passed

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

Running 20 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487102739355)
[PASS] testAuthInSources() (gas: 9223371487099292652)
[PASS] testBytecodeMatches() (gas: 4156670)
[PASS] testCastCost() (gas: 1394808)
[PASS] testChainlogValues() (gas: 9646611)
[PASS] testChainlogVersionBump() (gas: 3808996)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 4137320)
[PASS] testDirectSparkIntegration() (gas: 1750535)
[PASS] testFailNotScheduled() (gas: 14406)
[PASS] testFailTooEarly() (gas: 333389)
[PASS] testFailTooLate() (gas: 333789)
[PASS] testFailWrongDay() (gas: 333879)
[PASS] testGeneral() (gas: 38468048)
[PASS] testNewIlkRegistryValues() (gas: 1407967)
[PASS] testNextCastTime() (gas: 362560)
[PASS] testOnTime() (gas: 1382378)
[PASS] testPSMs() (gas: 2818355)
[PASS] testSparkLendParameterAdjustments() (gas: 1432457)
[PASS] testUseEta() (gas: 268710)
Test result: ok. 20 passed; 0 failed; finished in 1181.16s

Checklist

amusingaxl

Good to deploy.

rockyfour

LGTM to publish and archive

deploy params ok
license ok
execlib version ok
code matches repo

tests pass:

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

Running 21 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487102739355)
[PASS] testAuthInSources() (gas: 9223371487099292652)
[PASS] testBytecodeMatches() (gas: 4163432)
[PASS] testCastCost() (gas: 1478490)
[PASS] testChainlogValues() (gas: 9730293)
[PASS] testChainlogVersionBump() (gas: 3892656)
[PASS] testContractSize() (gas: 8940)
[PASS] testDeployCost() (gas: 4144060)
[PASS] testDirectSparkIntegration() (gas: 1834262)
[PASS] testFailNotScheduled() (gas: 14406)
[PASS] testFailTooEarly() (gas: 417592)
[PASS] testFailTooLate() (gas: 417549)
[PASS] testFailWrongDay() (gas: 417617)
[PASS] testGeneral() (gas: 38535704)
[PASS] testNewChainlogValues() (gas: 1515661)
[PASS] testNewIlkRegistryValues() (gas: 1491671)
[PASS] testNextCastTime() (gas: 446343)
[PASS] testOnTime() (gas: 1466105)
[PASS] testPSMs() (gas: 2902037)
[PASS] testSparkLendParameterAdjustments() (gas: 1519731)
[PASS] testUseEta() (gas: 352470)
Test result: ok. 21 passed; 0 failed; finished in 1113.76s

amusingaxl

Tests pass.

Good to handover and archive.

hexonaut · 2023-05-02T16:18:17Z

Sent spell to governance. Archive added.

rockyfour

Looks good to merge.

amusingaxl · 2023-05-02T18:30:23Z

Archive matches src
- make diff-archive-spell for current date or date="YYYY-MM-DD" make diff-archive-spell (date as per target exec date)
- ensure date corresponds to target exec date
Local Tests and CI PASS

amusingaxl

LGTM

add spark d3m spell

2a1117e

hexonaut requested review from gbalabasquer, amusingaxl and rockyfour April 26, 2023 08:38

hexonaut self-assigned this Apr 26, 2023

hexonaut added 3 commits April 26, 2023 18:02

add spell copy + hash

0d34561

add ilk registry check

9f62a90

add internal

40d4db4

Base automatically changed from 2023-04-21 to master April 27, 2023 14:42

hexonaut added 2 commits April 27, 2023 17:35

merge master

5540c11

zero out deployed spell and add prev spell

7efe500

amusingaxl reviewed Apr 28, 2023

View reviewed changes

rockyfour reviewed Apr 30, 2023

View reviewed changes

src/DssSpell.sol Show resolved Hide resolved

add explicit office hours

42f6434

rockyfour reviewed May 1, 2023

View reviewed changes

src/DssSpell.sol Outdated Show resolved Hide resolved

src/DssSpell.t.sol Outdated Show resolved Hide resolved

src/DssSpell.t.sol Show resolved Hide resolved

use chainlog and addr.addr for wbtc

7c92d12

rockyfour reviewed May 2, 2023

View reviewed changes

src/test/addresses_mainnet.sol Outdated Show resolved Hide resolved

change addresses order; re-add new chainlog values explicit checks

6217df5

rockyfour previously approved these changes May 2, 2023

View reviewed changes

amusingaxl previously approved these changes May 2, 2023

View reviewed changes

amusingaxl closed this May 2, 2023

add spell address and block/timestamp

b0dcfed

hexonaut reopened this May 2, 2023

hexonaut dismissed stale reviews from amusingaxl and rockyfour via b0dcfed May 2, 2023 12:25

rockyfour previously approved these changes May 2, 2023

View reviewed changes

amusingaxl previously approved these changes May 2, 2023

View reviewed changes

add archive

8e911ef

hexonaut dismissed stale reviews from amusingaxl and rockyfour via 8e911ef May 2, 2023 16:16

rockyfour approved these changes May 2, 2023

View reviewed changes

amusingaxl approved these changes May 2, 2023

View reviewed changes

hexonaut merged commit 99a1bf8 into master May 3, 2023

hexonaut deleted the 2023-04-28 branch May 3, 2023 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2023-04-28 - Spark D3M + Configuration Changes #339

2023-04-28 - Spark D3M + Configuration Changes #339

hexonaut commented Apr 26, 2023

amusingaxl left a comment •

edited by rockyfour

Loading

rockyfour left a comment •

edited

Loading

amusingaxl left a comment

rockyfour left a comment

amusingaxl left a comment

hexonaut commented May 2, 2023

rockyfour left a comment

amusingaxl commented May 2, 2023

amusingaxl left a comment

2023-04-28 - Spark D3M + Configuration Changes #339

2023-04-28 - Spark D3M + Configuration Changes #339

Conversation

hexonaut commented Apr 26, 2023

Description

Contribution Checklist

Checklist

amusingaxl left a comment • edited by rockyfour Loading

Choose a reason for hiding this comment

Checklist

rockyfour left a comment • edited Loading

Choose a reason for hiding this comment

Chain Security deployment report

Chain Security DAI Strategy and sDAI oracle

Mainnet Spell

Checklist

amusingaxl left a comment

Choose a reason for hiding this comment

rockyfour left a comment

Choose a reason for hiding this comment

amusingaxl left a comment

Choose a reason for hiding this comment

hexonaut commented May 2, 2023

rockyfour left a comment

Choose a reason for hiding this comment

amusingaxl commented May 2, 2023

amusingaxl left a comment

Choose a reason for hiding this comment

amusingaxl left a comment •

edited by rockyfour

Loading

rockyfour left a comment •

edited

Loading