diff --git a/lib/dss-test b/lib/dss-test index 4ad127cf5..36ff4adbc 160000 --- a/lib/dss-test +++ b/lib/dss-test @@ -1 +1 @@ -Subproject commit 4ad127cf53eeaddfb7b8ad56dd4b13e57d6a0067 +Subproject commit 36ff4adbcb35760614e0d2df864026991c23d028 diff --git a/src/DssSpell.t.base.sol b/src/DssSpell.t.base.sol index 0bc14e20a..f47ed2d88 100644 --- a/src/DssSpell.t.base.sol +++ b/src/DssSpell.t.base.sol @@ -260,17 +260,27 @@ contract DssSpellTestBase is Config, DssTest { } function _concat(string memory a, bytes32 b) internal pure returns (string memory) { - return string.concat(a, _bytes32ToStr(b)); + return string.concat(a, _bytes32ToString(b)); } - function _bytes32ToStr(bytes32 _bytes32) internal pure returns (string memory) { - bytes memory bytesArray = new bytes(32); - for (uint256 i; i < 32; i++) { + function _bytes32ToString(bytes32 _bytes32) internal pure returns (string memory) { + uint256 charCount = 0; + while(charCount < 32 && _bytes32[charCount] != 0) { + charCount++; + } + bytes memory bytesArray = new bytes(charCount); + for (uint256 i = 0; i < charCount; i++) { bytesArray[i] = _bytes32[i]; } return string(bytesArray); } + function _stringToBytes32(string memory source) internal pure returns (bytes32 result) { + assembly { + result := mload(add(source, 32)) + } + } + // 10^-5 (tenth of a basis point) as a RAY uint256 TOLERANCE = 10 ** 22; @@ -369,12 +379,6 @@ contract DssSpellTestBase is Config, DssTest { DssSpell(spell_).cast(); } - function _stringToBytes32(string memory source) internal pure returns (bytes32 result) { - assembly { - result := mload(add(source, 32)) - } - } - function _checkSystemValues(SystemValues storage values) internal { // dsr uint256 expectedDSRRate = rates.rates(values.pot_dsr); @@ -1484,63 +1488,47 @@ contract DssSpellTestBase is Config, DssTest { } } - function _skipWards(address target, address deployer) internal pure returns (bool) { - // Kept for consistency but in general we don't want to skip checking wards on mainnet - target; deployer; - return false; - } - + /** + * @dev Checks if the deployer of a contract has not kept `wards` access to the contract. + * Notice that it depends on `deployers` being kept up-to-date. + */ function _checkWards(address _addr, string memory contractName) internal { for (uint256 i = 0; i < deployers.count(); i ++) { address deployer = deployers.addr(i); - (bool ok, bytes memory data) = _addr.call( - abi.encodeWithSignature("wards(address)", deployer) - ); + (bool ok, bytes memory data) = _addr.call(abi.encodeWithSignature("wards(address)", deployer)); if (!ok || data.length != 32) return; + uint256 ward = abi.decode(data, (uint256)); if (ward > 0) { - if (_skipWards(_addr, deployer)) continue; - emit log("Error: Bad Auth"); emit log_named_address(" Deployer Address", deployer); emit log_named_string(" Affected Contract", contractName); - fail(); + fail("Error: Bad Auth"); } } } - function _checkSource(address _addr, string memory contractName) internal { - (bool ok, bytes memory data) = - _addr.call(abi.encodeWithSignature("src()")); + /** + * @dev Same as `_checkWards`, but for OSMs' underlying Median contracts. + */ + function _checkOsmSrcWards(address _addr, string memory contractName) internal { + (bool ok, bytes memory data) = _addr.call(abi.encodeWithSignature("src()")); if (!ok || data.length != 32) return; + address source = abi.decode(data, (address)); - string memory sourceName = string( - abi.encodePacked("source of ", contractName) - ); + string memory sourceName = _concat("src of ", contractName); _checkWards(source, sourceName); } - function _checkAuth(bool onlySource) internal { - _vote(address(spell)); - _scheduleWaitAndCast(address(spell)); - assertTrue(spell.done(), "TestError/spell-not-done"); - - bytes32[] memory contractNames = chainLog.list(); - for(uint256 i = 0; i < contractNames.length; i++) { - address _addr = chainLog.getAddress(contractNames[i]); - string memory contractName = string( - abi.encodePacked(contractNames[i]) - ); - if (onlySource) _checkSource(_addr, contractName); - else _checkWards(_addr, contractName); - } - } - - function _checkChainlogKey(bytes32 key) internal { - assertEq(chainLog.getAddress(key), addr.addr(key), _concat("TestError/Chainlog-key-mismatch-", key)); - } + /** + * @notice Checks if the the deployer of a contract the chainlog has not kept `wards` access to it. + * @dev Reverts if `key` is not in the chainlog. + */ + function _checkAuth(bytes32 key) internal { + address _addr = chainLog.getAddress(key); + string memory contractName = _bytes32ToString(key); - function _checkChainlogVersion(string memory key) internal { - assertEq(chainLog.version(), key, _concat("TestError/Chainlog-version-mismatch-", key)); + _checkWards(_addr, contractName); + _checkOsmSrcWards(_addr, contractName); } function _checkRWADocUpdate(bytes32 ilk, string memory currentDoc, string memory newDoc) internal { @@ -1820,52 +1808,175 @@ contract DssSpellTestBase is Config, DssTest { assertEq(actualHash, expectedHash); } - // Validate addresses in test harness match chainlog - function _testChainlogValues() internal { + struct ChainlogCache { + bytes32 versionHash; + bytes32 contentHash; + uint256 count; + bytes32[] keys; + address[] values; + } + + /** + * @dev Checks the integrity of the chainlog. + * This test case is able to catch the following spell issues: + + * 1. Modifications without version bumping: + * a. Removing a key. + * b. Updating a key. + * c. Adding a key. + * d. Removing a key and adding it back (this can change the order of the list). + * 2. Version bumping without modifications. + * 3. Dangling wards on new or updated keys. + * + * When adding or updating a key, the test will automatically check for dangling wards if applicable. + * Notice that when a key is removed, if it is not the last one, there is a side-effect of moving + * the last key to the position of the removed one (well-known Solidity iterability pattern). + * This will generate a false-positive that will cause the test to re-check wards for the moved key. + */ + function _testChainlogIntegrity() internal { + ChainlogCache memory cacheBefore = ChainlogCache({ + count: chainLog.count(), + keys: chainLog.list(), + versionHash: keccak256(abi.encodePacked("")), + contentHash: keccak256(abi.encode(new bytes32[](0), new address[](0))), + values: new address[](0) + }); + + cacheBefore.values = new address[](cacheBefore.count); + for(uint256 i = 0; i < cacheBefore.count; i++) { + cacheBefore.values[i] = chainLog.getAddress(cacheBefore.keys[i]); + } + + cacheBefore.versionHash = keccak256(abi.encodePacked(chainLog.version())); + // Using `abi.encode` to prevent ambiguous encoding + cacheBefore.contentHash = keccak256(abi.encode(cacheBefore.keys, cacheBefore.values)); + + ////////////////////////////////////////// + _vote(address(spell)); _scheduleWaitAndCast(address(spell)); assertTrue(spell.done()); - for(uint256 i = 0; i < chainLog.count(); i++) { - (bytes32 _key, address _val) = chainLog.get(i); - assertEq(_val, addr.addr(_key), _concat("TestError/chainlog-addr-mismatch-", _key)); + ////////////////////////////////////////// + + ChainlogCache memory cacheAfter = ChainlogCache({ + count: chainLog.count(), + keys: chainLog.list(), + versionHash: keccak256(abi.encodePacked("")), + contentHash: keccak256(abi.encode(new bytes32[](0), new address[](0))), + values: new address[](0) + }); + + cacheAfter.values = new address[](cacheAfter.count); + for(uint256 i = 0; i < cacheAfter.count; i++) { + cacheAfter.values[i] = chainLog.getAddress(cacheAfter.keys[i]); } - _checkChainlogVersion(afterSpell.chainlog_version); - } + cacheAfter.versionHash = keccak256(abi.encodePacked(chainLog.version())); + // Using `abi.encode` to prevent ambiguous encoding + cacheAfter.contentHash = keccak256(abi.encode(cacheAfter.keys, cacheAfter.values)); + + ////////////////////////////////////////// + + // If the version is the same, the content should not have changed + if (cacheAfter.versionHash == cacheBefore.versionHash) { + assertEq(cacheBefore.count, cacheAfter.count, "TestError/chainlog-version-not-updated-length-change"); + + // Add explicit check otherwise this would fail with an array-out-of-bounds error, + // since Foundry does not halt the execution when an assertion fails. + if (cacheBefore.count == cacheAfter.count) { + // Fail if the chainlog is the same size, but EITHER: + // 1. The value for a specific key changed + // 2. The order of keys changed + for (uint256 i = 0; i < cacheAfter.count; i++) { + assertEq( + cacheBefore.values[i], + cacheAfter.values[i], + _concat( + "TestError/chainlog-version-not-updated-value-change: ", + _concat( + _concat("+ ", cacheAfter.keys[i]), + _concat(" | - ", cacheBefore.keys[i]) + ) + ) + ); + } + } + } else { + // If the version changed, the content should have changed + assertTrue(cacheAfter.contentHash != cacheBefore.contentHash, "TestError/chainlog-version-updated-no-content-change"); + } + + // If the content has changed, we look into the diff + if (cacheAfter.contentHash != cacheBefore.contentHash) { + // If the content changed, the version should have changed + assertTrue(cacheAfter.versionHash != cacheBefore.versionHash, "TestError/chainlog-content-updated-no-version-change"); + + uint256 diffCount; + // Iteration must stop at the shorter array length + uint256 maxIters = cacheAfter.count > cacheBefore.count ? cacheBefore.count : cacheAfter.count; + + // Look for changes in existing keys + for (uint256 i = 0; i < maxIters; i++) { + if (cacheAfter.keys[i] != cacheBefore.keys[i]) { + // Change in order + diffCount += 1; + } else if (cacheAfter.values[i] != cacheBefore.values[i]) { + // Change in value + diffCount += 1; + } + } + + // Account for new keys + // Notice: we don't care about removed keys + if (cacheAfter.count > cacheBefore.count) { + diffCount += (cacheAfter.count - cacheBefore.count); + } + + //////////////////////////////////////// + + bytes32[] memory diffKeys = new bytes32[](diffCount); + uint256 j = 0; - // Ensure version is updated if chainlog changes - function _testChainlogVersionBump() internal { - uint256 _count = chainLog.count(); - string memory _version = chainLog.version(); - address[] memory _chainlog_addrs = new address[](_count); + for (uint256 i = 0; i < maxIters; i++) { + if (cacheAfter.keys[i] != cacheBefore.keys[i]) { + // Mark keys whose order has changed + diffKeys[j++] = cacheAfter.keys[i]; + } else if (cacheAfter.values[i] != cacheBefore.values[i]) { + // Mark changed values + diffKeys[j++] = cacheAfter.keys[i]; + } + } + + // Mark new keys + if (cacheAfter.count > cacheBefore.count) { + for (uint256 i = cacheBefore.count; i < cacheAfter.count; i++) { + diffKeys[j++] = cacheAfter.keys[i]; + } + } - for(uint256 i = 0; i < _count; i++) { - (, address _val) = chainLog.get(i); - _chainlog_addrs[i] = _val; + for (uint256 i = 0; i < diffKeys.length; i++) { + _checkAuth(diffKeys[i]); + } } + } + // Validate addresses in test harness match chainlog + function _testChainlogValues() internal { _vote(address(spell)); _scheduleWaitAndCast(address(spell)); assertTrue(spell.done()); - if (keccak256(abi.encodePacked(_version)) == keccak256(abi.encodePacked(chainLog.version()))) { - // Fail if the version is not updated and the chainlog count has changed - if (_count != chainLog.count()) { - emit log_named_string("Error", _concat("TestError/chainlog-version-not-updated-count-change-", _version)); - fail(); - return; - } - // Fail if the chainlog is the same size but local keys don't match the chainlog. - for(uint256 i = 0; i < _count; i++) { - (, address _val) = chainLog.get(i); - if (_chainlog_addrs[i] != _val) { - emit log_named_string("Error", _concat("TestError/chainlog-version-not-updated-address-change-", _version)); - fail(); - return; - } - } + bytes32[] memory keys = chainLog.list(); + for (uint256 i = 0; i < keys.length; i++) { + assertEq( + chainLog.getAddress(keys[i]), + addr.addr(keys[i]), + _concat("TestError/chainlog-vs-harness-key-mismatch: ", keys[i]) + ); } + + assertEq(chainLog.version(), afterSpell.chainlog_version, "TestError/chainlog-version-mismatch"); } function _checkCropCRVLPIntegration( @@ -1989,5 +2100,4 @@ contract DssSpellTestBase is Config, DssTest { // Dump all dai for next run vat.move(address(this), address(0x0), vat.dai(address(this))); } - } diff --git a/src/DssSpell.t.sol b/src/DssSpell.t.sol index 8651358c2..f5debac0b 100644 --- a/src/DssSpell.t.sol +++ b/src/DssSpell.t.sol @@ -95,24 +95,16 @@ contract DssSpellTest is DssSpellTestBase { _testUseEta(); } - function testAuth() public { - _checkAuth(false); - } - - function testAuthInSources() public { - _checkAuth(true); - } - function testBytecodeMatches() public { _testBytecodeMatches(); } - function testChainlogValues() public { - _testChainlogValues(); + function testChainlogIntegrity() public { + _testChainlogIntegrity(); } - function testChainlogVersionBump() public { - _testChainlogVersionBump(); + function testChainlogValues() public { + _testChainlogValues(); } // Leave this test public (for now) as this is acting like a config test @@ -176,49 +168,6 @@ contract DssSpellTest is DssSpellTestBase { // TESTS BELOW CAN BE ENABLED/DISABLED ON DEMAND - function testOsmAuth() private { // make private to disable - // address ORACLE_WALLET01 = 0x4D6fbF888c374D7964D56144dE0C0cFBd49750D3; - - // validate the spell does what we told it to - //bytes32[] memory ilks = reg.list(); - - //for(uint256 i = 0; i < ilks.length; i++) { - // uint256 class = reg.class(ilks[i]); - // if (class != 1) { continue; } - - // address pip = reg.pip(ilks[i]); - // // skip USDC, TUSD, PAXUSD, GUSD - // if (pip == 0x838212865E2c2f4F7226fCc0A3EFc3EB139eC661 || - // pip == 0x0ce19eA2C568890e63083652f205554C927a0caa || - // pip == 0xdF8474337c9D3f66C0b71d31C7D3596E4F517457 || - // pip == 0x57A00620Ba1f5f81F20565ce72df4Ad695B389d7) { - // continue; - // } - - // assertEq(OsmAbstract(pip).wards(ORACLE_WALLET01), 0); - //} - - //_vote(address(spell)); - //_scheduleWaitAndCast(address(spell)); - //assertTrue(spell.done()); - - //for(uint256 i = 0; i < ilks.length; i++) { - // uint256 class = reg.class(ilks[i]); - // if (class != 1) { continue; } - - // address pip = reg.pip(ilks[i]); - // // skip USDC, TUSD, PAXUSD, GUSD - // if (pip == 0x838212865E2c2f4F7226fCc0A3EFc3EB139eC661 || - // pip == 0x0ce19eA2C568890e63083652f205554C927a0caa || - // pip == 0xdF8474337c9D3f66C0b71d31C7D3596E4F517457 || - // pip == 0x57A00620Ba1f5f81F20565ce72df4Ad695B389d7) { - // continue; - // } - - // assertEq(OsmAbstract(pip).wards(ORACLE_WALLET01), 1); - //} - } - function testOracleList() private { // make private to disable // address ORACLE_WALLET01 = 0x4D6fbF888c374D7964D56144dE0C0cFBd49750D3; @@ -297,16 +246,6 @@ contract DssSpellTest is DssSpellTestBase { assertTrue(lerp.done()); } - function testNewChainlogValues() public { // make private to disable - _vote(address(spell)); - _scheduleWaitAndCast(address(spell)); - assertTrue(spell.done()); - - _checkChainlogKey("RWA009_A_INPUT_CONDUIT_URN_USDC"); - - _checkChainlogVersion("1.17.2"); - } - function testNewIlkRegistryValues() private { // make private to disable _vote(address(spell)); _scheduleWaitAndCast(address(spell));