Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: rely call detector [PoC] #382

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

feat: rely call detector [PoC] #382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d1d92f6
feat: adding a test to listen to rely events [PoC]
wei3erHase Jan 18, 2024
f301988
chore: comments
wei3erHase Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/dss-test
Open in desktop
Submodule dss-test updated 11 files
+24 −0 .github/workflows/tests.yaml
+1 −1 lib/forge-std
+123 −24 src/DssTest.sol
+75 −16 src/ScriptTools.sol
+2 −2 src/domains/Domain.sol
+2 −17 src/domains/OptimismDomain.sol
+4 −4 src/domains/RecordedLogs.sol
+51 −0 src/tests/FileUnitTest.t.sol
+2 −2 src/tests/ModifierUnitTest.t.sol
+22 −2 src/tests/ScriptToolsTest.t.sol
+1 −1 test.sh
195 changes: 10 additions & 185 deletions src/DssSpell.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,203 +20,28 @@ import "dss-exec-lib/DssExec.sol";
import "dss-exec-lib/DssAction.sol";
import { GemAbstract } from "dss-interfaces/ERC/GemAbstract.sol";

interface ProxyLike {
function exec(address target, bytes calldata args) external payable returns (bytes memory out);
}

interface RwaLiquidationLike {
function ilks(bytes32) external view returns (string memory, address, uint48, uint48);
function init(bytes32, uint256, string calldata, uint48) external;
}

interface VestLike {
function yank(uint256 _id) external;
}

contract DssSpellAction is DssAction {
// Provides a descriptive tag for bot consumption
// This should be modified weekly to provide a summary of the actions
// Hash: cast keccak -- "$(wget 'https://raw.githubusercontent.com/makerdao/community/7316e667dc358c624aa170776c0d6c8f0452dc36/governance/votes/Executive%20vote%20-%20January%2012%2C%202024.md' -q -O - 2>/dev/null)"
string public constant override description =
"2024-01-12 MakerDAO Executive Spell | Hash: 0xfa3538c5fd4e722c95a20215ecc483a80d9cb86dee89ae7df2c553bed2029882";
"Malicious spell";

// Set office hours according to the summary
function officeHours() public pure override returns (bool) {
return false;
}

// ---------- Rates ----------
// Many of the settings that change weekly rely on the rate accumulator
// described at https://docs.makerdao.com/smart-contract-modules/rates-module
// To check this yourself, use the following rate calculation (example 8%):
//
// $ bc -l <<< 'scale=27; e( l(1.08)/(60 * 60 * 24 * 365) )'
//
// A table of rates can be found at
// https://ipfs.io/ipfs/QmVp4mhhbwWGTfbh2BzwQB9eiBrQBKiqcPRZCaAxNUaar6
//
// uint256 internal constant X_PCT_RATE = ;

// ---------- Math ----------
uint256 constant internal MILLION = 10 ** 6;

// ----------- MKR transfer Addresses -----------

// Delegates
address internal constant DEFENSOR = 0x9542b441d65B6BF4dDdd3d4D2a66D8dCB9EE07a9;
address internal constant BONAPUBLICA = 0x167c1a762B08D7e78dbF8f24e5C3f1Ab415021D3;
address internal constant CLOAKY = 0x869b6d5d8FA7f4FFdaCA4D23FFE0735c5eD1F818;
address internal constant TRUENAME = 0x612F7924c367575a0Edf21333D96b15F1B345A5d;
address internal constant BLUE = 0xb6C09680D822F162449cdFB8248a7D3FC26Ec9Bf;
address internal constant UPMAKER = 0xbB819DF169670DC71A16F58F55956FE642cc6BcD;
address internal constant VIGILANT = 0x2474937cB55500601BCCE9f4cb0A0A72Dc226F61;
address internal constant JAG = 0x58D1ec57E4294E4fe650D1CB12b96AE34349556f;
address internal constant PBG = 0x8D4df847dB7FfE0B46AF084fE031F7691C6478c2;
address internal constant WBC = 0xeBcE83e491947aDB1396Ee7E55d3c81414fB0D47;
address internal constant NAVIGATOR = 0x11406a9CC2e37425F15f920F494A51133ac93072;
address internal constant PALC = 0x78Deac4F87BD8007b9cb56B8d53889ed5374e83A;

// BA Labs address
address internal constant RISK_WALLET_VEST = 0x5d67d5B1fC7EF4bfF31967bE2D2d7b9323c1521c;
// SES address
address internal constant SES_WALLET = 0x87AcDD9208f73bFc9207e1f6F0fDE906bcA95cc6;

// ---------- Launch Project Funds ----------
address internal constant LAUNCH_PROJECT_FUNDING = 0x3C5142F28567E6a0F172fd0BaaF1f2847f49D02F;

address internal immutable MCD_VEST_DAI = DssExecLib.getChangelogAddress("MCD_VEST_DAI");
address internal immutable MCD_VEST_DAI_LEGACY = DssExecLib.getChangelogAddress("MCD_VEST_DAI_LEGACY");
address internal immutable MIP21_LIQUIDATION_ORACLE = DssExecLib.getChangelogAddress("MIP21_LIQUIDATION_ORACLE");
GemAbstract internal immutable MKR = GemAbstract(DssExecLib.mkr());

// Function from https://github.com/makerdao/spells-goerli/blob/7d783931a6799fe8278e416b5ac60d4bb9c20047/archive/2022-11-14-DssSpell/Goerli-DssSpell.sol#L59
function _updateDoc(bytes32 ilk, string memory doc) internal {
( , address pip, uint48 tau, ) = RwaLiquidationLike(MIP21_LIQUIDATION_ORACLE).ilks(ilk);
require(pip != address(0), "DssSpell/unexisting-rwa-ilk");

// Init the RwaLiquidationOracle to reset the doc
RwaLiquidationLike(MIP21_LIQUIDATION_ORACLE).init(
ilk, // ilk to update
0, // price ignored if init() has already been called
doc, // new legal document
tau // old tau value
);
}

// ---------- Trigger Spark Proxy Spell ----------
// Spark Proxy: https://github.com/marsfoundation/sparklend/blob/d42587ba36523dcff24a4c827dc29ab71cd0808b/script/output/1/primary-sce-latest.json#L2
address internal constant SPARK_PROXY = 0x3300f198988e4C9C63F75dF86De36421f06af8c4;
address internal constant SPARK_SPELL = 0x2f2c514137173bc98B3699A0d291f7593637c596;
address constant TO_RELY = address(69);

function actions() public override {

// ---------- November Delegate Compensation ----------
// Forum: https://forum.makerdao.com/t/november-2023-aligned-delegate-compensation/23351

// 0xDefensor - 41.67 MKR - 0x9542b441d65B6BF4dDdd3d4D2a66D8dCB9EE07a9
MKR.transfer(DEFENSOR, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// BONAPUBLICA - 41.67 MKR - 0x167c1a762B08D7e78dbF8f24e5C3f1Ab415021D3
MKR.transfer(BONAPUBLICA, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// Cloaky - 41.67 MKR - 0x869b6d5d8FA7f4FFdaCA4D23FFE0735c5eD1F818
MKR.transfer(CLOAKY, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// TRUE NAME - 41.67 MKR - 0x612F7924c367575a0Edf21333D96b15F1B345A5d
MKR.transfer(TRUENAME, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// BLUE - 13.95 MKR - 0xb6C09680D822F162449cdFB8248a7D3FC26Ec9Bf
MKR.transfer(BLUE, 13.95 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// UPMaker - 13.89 MKR - 0xbB819DF169670DC71A16F58F55956FE642cc6BcD
MKR.transfer(UPMAKER, 13.89 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// vigilant - 13.89 MKR - 0x2474937cB55500601BCCE9f4cb0A0A72Dc226F61
MKR.transfer(VIGILANT, 13.89 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// JAG - 13.02 MKR - 0x58D1ec57E4294E4fe650D1CB12b96AE34349556f
MKR.transfer(JAG, 13.02 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// PBG - 0.45 MKR - 0x8D4df847dB7FfE0B46AF084fE031F7691C6478c2
MKR.transfer(PBG, 0.45 ether); // NOTE: ether is a keyword helper, only MKR is transferred here

// ---------- December Delegate Compensation ----------
// Forum: https://forum.makerdao.com/t/december-2023-aligned-delegate-compensation/23352

// 0xDefensor - 41.67 MKR - 0x9542b441d65B6BF4dDdd3d4D2a66D8dCB9EE07a9
MKR.transfer(DEFENSOR, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// BONAPUBLICA - 41.67 MKR - 0x167c1a762B08D7e78dbF8f24e5C3f1Ab415021D3
MKR.transfer(BONAPUBLICA, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// Cloaky - 41.67 MKR - 0x869b6d5d8FA7f4FFdaCA4D23FFE0735c5eD1F818
MKR.transfer(CLOAKY, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// TRUE NAME - 41.67 MKR - 0x612F7924c367575a0Edf21333D96b15F1B345A5d
MKR.transfer(TRUENAME, 41.67 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// BLUE - 39.20 MKR - 0xb6C09680D822F162449cdFB8248a7D3FC26Ec9Bf
MKR.transfer(BLUE, 39.20 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// PBG - 13.89 MKR - 0x8D4df847dB7FfE0B46AF084fE031F7691C6478c2
MKR.transfer(PBG, 13.89 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// UPMaker - 13.89 MKR - 0xbB819DF169670DC71A16F58F55956FE642cc6BcD
MKR.transfer(UPMAKER, 13.89 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// vigilant - 13.89 MKR - 0x2474937cB55500601BCCE9f4cb0A0A72Dc226F61
MKR.transfer(VIGILANT, 13.89 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// JAG - 12.95 MKR - 0x58D1ec57E4294E4fe650D1CB12b96AE34349556f
MKR.transfer(JAG, 12.95 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// WBC - 11.28 MKR - 0xeBcE83e491947aDB1396Ee7E55d3c81414fB0D47
MKR.transfer(WBC, 11.28 ether); // NOTE: ether is a keyword helper, only MKR is transferred here

// ---------- Offboarded Delegate Buffer Payments ----------
// Forum: https://forum.makerdao.com/t/october-2023-aligned-delegate-compensation/22732#october-compensation-2

// Navigator - 20.84 MKR - 0x11406a9CC2e37425F15f920F494A51133ac93072
MKR.transfer(NAVIGATOR, 20.84 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// PALC - 6.95 MKR - 0x78Deac4F87BD8007b9cb56B8d53889ed5374e83A
MKR.transfer(PALC, 6.95 ether); // NOTE: ether is a keyword helper, only MKR is transferred here

// ---------- yank Dai streams ----------

// yank Dai stream 21 - DECO
// Forum: https://forum.makerdao.com/t/mip39c3-sp12-core-unit-offboarding-deco/22333
VestLike(MCD_VEST_DAI_LEGACY).yank(21);

// yank Dai stream 15 - SES
// Forum: https://forum.makerdao.com/t/mip39c3-sp11-core-unit-offboarding-ses/22332
VestLike(MCD_VEST_DAI).yank(15);

// ---------- CU MKR payments ----------
// MIP: https://mips.makerdao.com/mips/details/MIP40c3SP25

// BA Labs - 175.00 MKR - 0x5d67d5B1fC7EF4bfF31967bE2D2d7b9323c1521c
MKR.transfer(RISK_WALLET_VEST, 175.00 ether); // NOTE: ether is a keyword helper, only MKR is transferred here
// SES - 508.55 MKR - 0x87AcDD9208f73bFc9207e1f6F0fDE906bcA95cc6
MKR.transfer(SES_WALLET, 508.55 ether); // NOTE: ether is a keyword helper, only MKR is transferred here

// ---------- Launch Project Funding ----------
// Forum: https://forum.makerdao.com/t/utilization-of-the-launch-project-under-the-accessibility-scope/21468/9

// Launch Project - 4,500,000.00 DAI - 0x3C5142F28567E6a0F172fd0BaaF1f2847f49D02F
DssExecLib.sendPaymentFromSurplusBuffer(LAUNCH_PROJECT_FUNDING, 4_500_000);
// Launch Project - 820.00 MKR - 0x3C5142F28567E6a0F172fd0BaaF1f2847f49D02F
MKR.transfer(LAUNCH_PROJECT_FUNDING, 820.00 ether); // NOTE: ether is a keyword helper, only MKR is transferred here

// ---------- Update doc parameter ----------
// Forum: https://forum.makerdao.com/t/rwa009-hvbank-mip21-token-ces-domain-team-assessment/15861/14

// Update HVBank (RWA009-A) doc to QmfEgZuiw6wsTRUYerdPZNUrqDXSGM6Nm4fM3nG7nNbEjT
_updateDoc("RWA009-A", "QmfEgZuiw6wsTRUYerdPZNUrqDXSGM6Nm4fM3nG7nNbEjT");

// ---------- Spark D3M line increase ----------
// Forum: https://forum.makerdao.com/t/spark-spell-proposed-changes/23298
// Poll: https://vote.makerdao.com/polling/QmdQSuAc

// Increase the line by 400 million from 800 million to 1.2 billion Dai
DssExecLib.setIlkAutoLineDebtCeiling("DIRECT-SPARK-DAI", 1200 * MILLION);

// ---------- Trigger Spark Proxy Spell ----------
// Forum: https://forum.makerdao.com/t/spark-spell-proposed-changes/23298
// Forum: https://forum.makerdao.com/t/jan-11th-2024-expedited-inclusion-of-a-patch-to-spark-pool-implementation/23393
// Poll: https://vote.makerdao.com/polling/QmdVy1Uk
// Poll: https://vote.makerdao.com/polling/QmeWioX1
// Poll: https://vote.makerdao.com/polling/QmRdew4b
// Poll: https://vote.makerdao.com/polling/QmXtvu32
// Poll: https://vote.makerdao.com/polling/QmRKkMnx

// Activate Spark Proxy Spell - 0x2f2c514137173bc98B3699A0d291f7593637c596
ProxyLike(SPARK_PROXY).exec(SPARK_SPELL, abi.encodeWithSignature("execute()"));
Reliable(T0_RELY).rely(address(420));
// Reliable(TO_RELY).rely(address(420));
}
}

interface Reliable {
function rely(address) external;
}

contract DssSpell is DssExec {
constructor() DssExec(block.timestamp + 30 days, address(new DssSpellAction())) {}
}

address constant T0_RELY = 0x35D1b3F3D7966A1DFe207aa4514C12a259A0492B;
46 changes: 22 additions & 24 deletions src/DssSpell.t.base.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,30 +317,28 @@ contract DssSpellTestBase is Config, DssTest {
spellValues.deployed_spell_created = spellValues.deployed_spell != address(0)
? spellValues.deployed_spell_created
: block.timestamp;
spell = spellValues.deployed_spell != address(0)
? DssSpell(spellValues.deployed_spell)
: new DssSpell();

if (spellValues.deployed_spell_block != 0 && spell.eta() != 0) {
// if we have a deployed spell in the config
// we want to roll our fork to the block where it was deployed
// this means the test suite will continue to accurately pass/fail
// even if mainnet has already scheduled/cast the spell
vm.makePersistent(address(rates));
vm.makePersistent(address(addr));
vm.makePersistent(address(deployers));
vm.makePersistent(address(wallets));
vm.rollFork(spellValues.deployed_spell_block);

// Reset `eta` to `0`, otherwise the tests will fail with "This spell has already been scheduled".
// This is a workaround for the issue described here:
// @see { https://github.com/foundry-rs/foundry/issues/5739 }
vm.store(
address(spell),
bytes32(0),
bytes32(0)
);
}
spell = new DssSpell();

// if (spellValues.deployed_spell_block != 0 && spell.eta() != 0) {
// // if we have a deployed spell in the config
// // we want to roll our fork to the block where it was deployed
// // this means the test suite will continue to accurately pass/fail
// // even if mainnet has already scheduled/cast the spell
// vm.makePersistent(address(rates));
// vm.makePersistent(address(addr));
// vm.makePersistent(address(deployers));
// vm.makePersistent(address(wallets));
// vm.rollFork(spellValues.deployed_spell_block);

// // Reset `eta` to `0`, otherwise the tests will fail with "This spell has already been scheduled".
// // This is a workaround for the issue described here:
// // @see { https://github.com/foundry-rs/foundry/issues/5739 }
// vm.store(
// address(spell),
// bytes32(0),
// bytes32(0)
// );
// }
}

function _vote(address spell_) internal {
Expand Down
52 changes: 52 additions & 0 deletions src/DssSpell.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,58 @@ contract DssSpellTest is DssSpellTestBase {
_checkAuth(false);
}

event LogNote(
bytes4 indexed sig,
address indexed usr,
bytes32 indexed arg1,
bytes32 indexed arg2,
bytes data
) anonymous;

/**
* NOTE: this test PoC will detect if the spell is making a `rely` call
* - needs to be disabled when actually wanting to make a `rely` call
* - foundry doesn't admit `expectEmit(n)`
* - should add a test for Rely(address) event (new contracts design)
* - should add same mechanics for `deny` calls
*/
function testFailAuthCalls() public { // to be run when NO rely calls are expected
_vote(address(spell));
spell.schedule();
vm.warp(spell.nextCastTime());

// this test should be disabled when making a `rely` call
// the crafter will then need to enable the following test and declare all rely calls
// either this or the following tests must be run
// cannot join 2 tests because this one requires `testFail` and the following `test`
vm.expectEmit(false, false, false, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we validate at least topic 1 here?

Suggested change
vm.expectEmit(false, false, false, false);
vm.expectEmit(true, false, false, false);

This way, it would only fail if rely is called somewhere, otherwise, I guess any LogNote would trigger a failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also we do have some contracts that use proper Rely(address) events instead of LibNote, so this test would have to cover that as well.

Copy link
Contributor Author

@wei3erHase wei3erHase Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

another test needs to cover it, same structure, only expecting emit a Rely(address) (we cannot put both in the same test), as the expectation would only fail if BOTH the expectations are fulfilled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the 1st topic is ALWAYS validated, the 2nd topic (which corresponds to the 1st boolean) would be:

  • for Note: address indexed usr
  • for Rely(address): address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

otherwise, I guess any LogNote would trigger a failure

beware that LogNote is an anonymous event, meaning, it does NOT have a topic 0 == keccak(LogNote), the topic0 IS the bytes4 indexed sig

// 0x65fae35e is signature for "rely(address)"
emit LogNote(hex"65fae35e", address(0), bytes32(0), bytes32(0), "");

spell.cast();
assertTrue(spell.done(), "TestError/spell-not-done");
}

function testAuthCalls() public { // to be run when rely calls are expected, doesn't fail w/ alien calls
_vote(address(spell));
spell.schedule();
vm.warp(spell.nextCastTime());

// when this test is run, a checklist item "check logs bloom for rely(address)" should be added
// the crafter needs to declare here all the addresses expected to be called with rely(address)
// the reviewer will check the logs bloom and verify that there is not an alien address
vm.expectEmit(true, false, false, false, address(69));
// `rely(address)` topic: 0x65fae35e00000000000000000000000000000000000000000000000000000000
emit LogNote(hex"65fae35e", address(420), bytes32(0), bytes32(0), "");

spell.cast();
assertTrue(spell.done(), "TestError/spell-not-done");
}

function testFailAlienAuthCalls() private {
// not possible w/o `expectEmit(n)` or generic target `vm.expectCall(n)` for now
}

function testAuthInSources() public {
_checkAuth(true);
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/config.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ contract Config {
// Values for spell-specific parameters
//
spellValues = SpellValues({
deployed_spell: address(0x54561C3BC933e3B61bD44eb402AeD19AC72b4DA0), // populate with deployed spell if deployed
deployed_spell_created: 1705012463, // use `make deploy-info tx=<deployment-tx>` to obtain the timestamp
deployed_spell: address(0), // populate with deployed spell if deployed
deployed_spell_created: 0, // use `make deploy-info tx=<deployment-tx>` to obtain the timestamp
deployed_spell_block: 18986683, // use `make deploy-info tx=<deployment-tx>` to obtain the block number
previous_spells: prevSpells, // older spells to ensure are executed first
office_hours_enabled: false, // true if officehours is expected to be enabled in the spell
Expand Down
Loading