From 485520f0c4d5590b4e621bf33b4329854b49bbb6 Mon Sep 17 00:00:00 2001 From: Chris <104409744+vreff@users.noreply.github.com> Date: Wed, 30 Aug 2023 12:20:18 -0400 Subject: [PATCH] Add access control and extra tests --- .../automation/upkeeps/MercuryRegistry.sol | 40 +++++++++------ .../foundry/automation/MercuryRegistry.t.sol | 51 +++++++++++++++++-- 2 files changed, 71 insertions(+), 20 deletions(-) diff --git a/contracts/src/v0.8/dev/automation/upkeeps/MercuryRegistry.sol b/contracts/src/v0.8/dev/automation/upkeeps/MercuryRegistry.sol index 7098dda300a..83ea301d65e 100644 --- a/contracts/src/v0.8/dev/automation/upkeeps/MercuryRegistry.sol +++ b/contracts/src/v0.8/dev/automation/upkeeps/MercuryRegistry.sol @@ -1,5 +1,6 @@ pragma solidity 0.8.6; +import "../../../shared/access/ConfirmedOwner.sol"; import "../../../automation/interfaces/AutomationCompatibleInterface.sol"; import "../2_1/interfaces/FeedLookupCompatibleInterface.sol"; import "../../../ChainSpecificUtil.sol"; @@ -22,12 +23,12 @@ import "../../../ChainSpecificUtil.sol"; | - `MercuryRegistry.t.sol` - contains foundry tests to demonstrate various flows. | | | | TODO: | -| - Access control. Specifically, the the ability to execute `performUpkeep`. | | - Optimize gas consumption. | -+---------------------------------------------------------------------------------------------------------------------*/ -contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleInterface { +contract MercuryRegistry is ConfirmedOwner, AutomationCompatibleInterface, FeedLookupCompatibleInterface { error DuplicateFeed(string feedId); error FeedNotActive(string feedId); + error StaleReport(string feedId, uint32 currentTimestamp, uint32 incomingTimestamp); // Feed object used for storing feed data. // not included but contained in reports: @@ -60,6 +61,8 @@ contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleI event FeedUpdated(uint32 observationsTimestamp, int192 price, int192 bid, int192 ask, string feedId); + uint32 private constant MIN_GAS_FOR_PERFORM = 200_000; + string constant c_feedParamKey = "feedIdHex"; // for Mercury v0.2 - format by which feeds are identified string constant c_timeParamKey = "blockNumber"; // for Mercury v0.2 - format by which feeds are filtered to be sufficiently recent IVerifierProxy public s_verifier; // for Mercury v0.2 - verifies off-chain reports @@ -77,7 +80,7 @@ contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleI address verifier, int192 deviationPercentagePPM, uint32 stalenessSeconds - ) { + ) ConfirmedOwner(msg.sender) { s_verifier = IVerifierProxy(verifier); // Store desired deviation threshold and staleness seconds. @@ -142,22 +145,22 @@ contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleI } // Use deviated off-chain values to update on-chain state. - // TODO: - // - The implementation provided here is readable but crude. Remaining gas should be checked between iterations - // of the for-loop, and the failure of a single item should not cause the entire batch to revert. function performUpkeep(bytes calldata performData) external override { (bytes[] memory values /* bytes memory lookupData */, ) = abi.decode(performData, (bytes[], bytes)); for (uint256 i = 0; i < values.length; i++) { - // Verify and decode report. + // Verify and decode the Mercury report. Report memory report = abi.decode(s_verifier.verify(values[i]), (Report)); string memory feedId = bytes32ToHexString(abi.encodePacked(report.feedId)); // Feeds that have been removed between checkUpkeep and performUpkeep should not be updated. - require(bytes(s_feedMapping[feedId].feedId).length > 0, "feed removed"); + if (!s_feedMapping[feedId].active) { + revert FeedNotActive(feedId); + } - // Sanity check. Stale reports should not get through, but ensure they do not cause a regression - // in the registry. - require(s_feedMapping[feedId].observationsTimestamp <= report.observationsTimestamp, "stale report"); + // Ensure stale reports do not cause a regression in the registry. + if (s_feedMapping[feedId].observationsTimestamp > report.observationsTimestamp) { + revert StaleReport(feedId, s_feedMapping[feedId].observationsTimestamp, report.observationsTimestamp); + } // Assign new values to state. s_feedMapping[feedId].bid = report.bid; @@ -165,8 +168,13 @@ contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleI s_feedMapping[feedId].price = report.price; s_feedMapping[feedId].observationsTimestamp = report.observationsTimestamp; - // Emit log (not gas efficient to do this for each update). + // Emit log. emit FeedUpdated(report.observationsTimestamp, report.price, report.bid, report.ask, feedId); + + // Ensure enough gas remains for the next iteration. Otherwise, stop here. + if (gasleft() < MIN_GAS_FOR_PERFORM) { + return; + } } } @@ -212,7 +220,7 @@ contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleI return string(abi.encodePacked("0x", converted)); } - function addFeeds(string[] memory feedIds, string[] memory feedNames) external { + function addFeeds(string[] memory feedIds, string[] memory feedNames) external onlyOwner { for (uint256 i = 0; i < feedIds.length; i++) { string memory feedId = feedIds[i]; if (s_feedMapping[feedId].active) { @@ -227,7 +235,7 @@ contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleI } } - function setFeeds(string[] memory feedIds, string[] memory feedNames) public { + function setFeeds(string[] memory feedIds, string[] memory feedNames) public onlyOwner { // Ensure correctly formatted constructor arguments. require(feedIds.length == feedNames.length, "incorrectly formatted feeds"); @@ -250,12 +258,12 @@ contract MercuryRegistry is AutomationCompatibleInterface, FeedLookupCompatibleI s_feeds = feedIds; } - function setConfig(int192 deviationPercentagePPM, uint32 stalenessSeconds) external { + function setConfig(int192 deviationPercentagePPM, uint32 stalenessSeconds) external onlyOwner { s_stalenessSeconds = stalenessSeconds; s_deviationPercentagePPM = deviationPercentagePPM; } - function setVerifier(address verifier) external { + function setVerifier(address verifier) external onlyOwner { s_verifier = IVerifierProxy(verifier); } } diff --git a/contracts/test/v0.8/foundry/automation/MercuryRegistry.t.sol b/contracts/test/v0.8/foundry/automation/MercuryRegistry.t.sol index c6607f11747..cb4dba98cbc 100644 --- a/contracts/test/v0.8/foundry/automation/MercuryRegistry.t.sol +++ b/contracts/test/v0.8/foundry/automation/MercuryRegistry.t.sol @@ -1,7 +1,6 @@ pragma solidity ^0.8.0; import {Test} from "forge-std/Test.sol"; -import {console} from "forge-std/console.sol"; import "../../../../src/v0.8/dev/automation/upkeeps/MercuryRegistry.sol"; import "../../../../src/v0.8/dev/automation/upkeeps/MercuryRegistryBatchUpkeep.sol"; import "../../../../src/v0.8/dev/automation/2_1/interfaces/FeedLookupCompatibleInterface.sol"; @@ -42,6 +41,12 @@ contract MercuryRegistryTest is Test { bytes s_august23ETHUSDMercuryReport = hex"0006c41ec94138ae62cce3f1a2b852e42fe70359502fa7b6bdbf81207970d88e00000000000000000000000000000000000000000000000000000000016d874d000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000220000000000000000000000000000000000000000000000000000000000000028000010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000120f753e1201d54ac94dfd9334c542562ff7e42993419a661261d010af0cbfd4e340000000000000000000000000000000000000000000000000000000064e66415000000000000000000000000000000000000000000000000000000275dbe6079000000000000000000000000000000000000000000000000000000275c905eba000000000000000000000000000000000000000000000000000000275e5693080000000000000000000000000000000000000000000000000000000002286ce7c44fa27f67f6dd0a8bb40c12f0f050231845789f022a82aa5f4b3fe5bf2068fb0000000000000000000000000000000000000000000000000000000002286ce70000000000000000000000000000000000000000000000000000000064e664150000000000000000000000000000000000000000000000000000000000000002a2b01f7741563cfe305efaec43e56cd85731e3a8e2396f7c625bd16adca7b39c97805b6170adc84d065f9d68c87104c3509aeefef42c0d1711e028ace633888000000000000000000000000000000000000000000000000000000000000000025d984ad476bda9547cf0f90d32732dc5a0d84b0e2fe9795149b786fb05332d4c092e278b4dddeef45c070b818c6e221db2633b573d616ef923c755a145ea099c"; + // Feed: USDC/USD + // Date: Wednesday, August 30, 2023 5:05:01 PM + // Price: $1.00035464 + bytes s_august30USDCUSDMercuryReport = + hex"0006970c13551e2a390246f5eccb62b9be26848e72026830f4688f49201b5a050000000000000000000000000000000000000000000000000000000001c89843000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000220000000000000000000000000000000000000000000000000000000000000028000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000120a5b07943b89e2c278fc8a2754e2854316e03cb959f6d323c2d5da218fb6b0ff80000000000000000000000000000000000000000000000000000000064ef69fa0000000000000000000000000000000000000000000000000000000005f5da000000000000000000000000000000000000000000000000000000000005f5b0f80000000000000000000000000000000000000000000000000000000005f5f8b0000000000000000000000000000000000000000000000000000000000240057307d0a0421d25328cb6dcfc5d0e211ff0580baaaf104e9877fc52cf2e8ec0aa7d00000000000000000000000000000000000000000000000000000000024005730000000000000000000000000000000000000000000000000000000064ef69fa0000000000000000000000000000000000000000000000000000000000000002b9e7fb46f1e9d22a1156024dc2bbf2bc6d337e0a2d78aaa3fb6e43b880217e5897732b516e39074ef4dcda488733bfee80c0a10714b94621cd93df6842373cf5000000000000000000000000000000000000000000000000000000000000000205ca5f8da9d6ae01ec6d85c681e536043323405b3b8a15e4d2a288e02dac32f10b2294593e270a4bbf53b0c4978b725293e85e49685f1d3ce915ff670ab6612f"; + function setUp() public virtual { // Set owner, and fork Arbitrum Goerli Testnet (chain ID 421613). // A public Arbitrum Goerli RPC url is being used, and the fork should be cached in CI so availability is not an issue for test runs. @@ -121,6 +126,10 @@ contract MercuryRegistryTest is Test { assertEq(localFeedId, s_BTCUSDFeedId); assertEq(active, true); + // Save this for later in the test. + bytes memory oldPerformData = performData; + uint32 oldObservationsTimestamp = observationsTimestamp; + // Obtain mercury report off-chain (for August 23 BTC/USD price & ETH/USD price) values = new bytes[](2); values[0] = s_august23BTCUSDMercuryReport; @@ -131,7 +140,6 @@ contract MercuryRegistryTest is Test { assertEq(shouldPerformUpkeep, true); // Perform upkeep to update on-chain state. - console.logBytes(performData); s_testRegistry.performUpkeep(performData); // Make a batch request for both the BTC/USD feed data and the ETH/USD feed data. @@ -162,6 +170,29 @@ contract MercuryRegistryTest is Test { // Pass the obtained mercury report into checkCallback, to assert that an update is not warranted. (shouldPerformUpkeep, performData) = s_testRegistry.checkCallback(values, bytes("LOOKUP_DATA")); assertEq(shouldPerformUpkeep, false); + + // Ensure stale reports cannot be included. + vm.expectRevert( + abi.encodeWithSelector( + MercuryRegistry.StaleReport.selector, + feedIds[0], + feeds[0].observationsTimestamp, + oldObservationsTimestamp + ) + ); + s_testRegistry.performUpkeep(oldPerformData); + + // Ensure reports for inactive feeds cannot be included. + bytes[] memory inactiveFeedReports = new bytes[](1); + inactiveFeedReports[0] = s_august30USDCUSDMercuryReport; + bytes memory lookupData = ""; + vm.expectRevert( + abi.encodeWithSelector( + MercuryRegistry.FeedNotActive.selector, + "0xa5b07943b89e2c278fc8a2754e2854316e03cb959f6d323c2d5da218fb6b0ff8" // USDC/USD feed id + ) + ); + s_testRegistry.performUpkeep(abi.encode(inactiveFeedReports, lookupData)); } // Below are the same tests as `testMercuryRegistry`, except done via a batching Mercury registry that @@ -226,8 +257,8 @@ contract MercuryRegistryTest is Test { (shouldPerformUpkeep, performData) = batchedRegistry.checkCallback(values, bytes("LOOKUP_DATA")); assertEq(shouldPerformUpkeep, true); - // Perform upkeep to update on-chain state. - batchedRegistry.performUpkeep(performData); + // Perform upkeep to update on-chain state, but with not enough gas to update both feeds. + batchedRegistry.performUpkeep{gas: 250_000}(performData); // Make a batch request for both the BTC/USD feed data and the ETH/USD feed data. MercuryRegistry.Feed[] memory feeds = s_testRegistry.getLatestFeedData(feedIds); @@ -240,6 +271,18 @@ contract MercuryRegistryTest is Test { assertEq(feeds[0].feedName, "BTC/USD"); assertEq(feeds[0].feedId, s_BTCUSDFeedId); + // Check state of ETH/USD feed to observe that the update was not propagated. + assertEq(feeds[1].observationsTimestamp, 0); + assertEq(feeds[1].bid, 0); + assertEq(feeds[1].price, 0); + assertEq(feeds[1].ask, 0); + assertEq(feeds[1].feedName, "ETH/USD"); + assertEq(feeds[1].feedId, s_ETHUSDFeedId); + + // Try again, with sufficient gas to update both feeds. + batchedRegistry.performUpkeep{gas: 2_500_000}(performData); + feeds = s_testRegistry.getLatestFeedData(feedIds); + // Check state of ETH/USD feed to ensure update was propagated. assertEq(feeds[1].observationsTimestamp, 1692820501); // Wednesday, August 23, 2023 7:55:01 PM assertEq(feeds[1].bid, 169056689850); // $1,690.56689850