From deef73595955451dbb4bfa6f404cab44ffb83a7b Mon Sep 17 00:00:00 2001 From: blockgroot <170620375+blockgroot@users.noreply.github.com> Date: Tue, 5 Nov 2024 11:05:55 +0530 Subject: [PATCH] Upgrade StaderOracle implementation (#258) * forge install: openzeppelin-foundry-upgrades v0.3.6 * feat: change min ONO requirement * chore: update equality check to more broader * feat: update consensus of sd price match --- .gitmodules | 3 ++ contracts/StaderOracle.sol | 8 ++-- lib/openzeppelin-foundry-upgrades | 1 + test/foundry_tests/StaderOracle.t.sol | 66 ++++++++++++--------------- 4 files changed, 39 insertions(+), 39 deletions(-) create mode 160000 lib/openzeppelin-foundry-upgrades diff --git a/.gitmodules b/.gitmodules index 888d42dc..7d79667c 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,3 +1,6 @@ [submodule "lib/forge-std"] path = lib/forge-std url = https://github.com/foundry-rs/forge-std +[submodule "lib/openzeppelin-foundry-upgrades"] + path = lib/openzeppelin-foundry-upgrades + url = https://github.com/OpenZeppelin/openzeppelin-foundry-upgrades diff --git a/contracts/StaderOracle.sol b/contracts/StaderOracle.sol index bec4b6e2..60f9f0c2 100644 --- a/contracts/StaderOracle.sol +++ b/contracts/StaderOracle.sol @@ -29,7 +29,7 @@ contract StaderOracle is IStaderOracle, AccessControlUpgradeable, PausableUpgrad uint256 public constant MAX_ER_UPDATE_FREQUENCY = 7200 * 7; // 7 days uint256 public constant ER_CHANGE_MAX_BPS = 10_000; uint256 public override erChangeLimit; - uint256 public constant MIN_TRUSTED_NODES = 5; + uint256 public constant MIN_TRUSTED_NODES = 3; uint256 public override trustedNodeChangeCoolingPeriod; /// @inheritdoc IStaderOracle @@ -119,6 +119,9 @@ contract StaderOracle is IStaderOracle, AccessControlUpgradeable, PausableUpgrad if (block.number < lastTrustedNodeCountChangeBlock + trustedNodeChangeCoolingPeriod) { revert CooldownNotComplete(); } + if (trustedNodesCount <= MIN_TRUSTED_NODES) { + revert InsufficientTrustedNodes(); + } lastTrustedNodeCountChangeBlock = block.number; isTrustedNode[_nodeAddress] = false; @@ -304,8 +307,7 @@ contract StaderOracle is IStaderOracle, AccessControlUpgradeable, PausableUpgrad // Emit SD Price submitted event emit SDPriceSubmitted(msg.sender, _sdPriceData.sdPriceInETH, _sdPriceData.reportingBlockNumber, block.number); - // price can be derived once more than 66% percent oracles have submitted price - if ((submissionCount >= (2 * trustedNodesCount) / 3 + 1)) { + if ((submissionCount >= trustedNodesCount / 2 + 1)) { lastReportedSDPriceData = _sdPriceData; lastReportedSDPriceData.sdPriceInETH = getMedianValue(sdPrices); diff --git a/lib/openzeppelin-foundry-upgrades b/lib/openzeppelin-foundry-upgrades new file mode 160000 index 00000000..16e0ae21 --- /dev/null +++ b/lib/openzeppelin-foundry-upgrades @@ -0,0 +1 @@ +Subproject commit 16e0ae21e0e39049f619f2396fa28c57fad07368 diff --git a/test/foundry_tests/StaderOracle.t.sol b/test/foundry_tests/StaderOracle.t.sol index b818690d..bb5f2c30 100644 --- a/test/foundry_tests/StaderOracle.t.sol +++ b/test/foundry_tests/StaderOracle.t.sol @@ -125,6 +125,7 @@ contract StaderOracleTest is Test { } function test_add_remove_trustedNode() public { + // Tests for adding nodes address trustedNode = vm.addr(123); assertEq(staderOracle.trustedNodesCount(), 0); assertFalse(staderOracle.isTrustedNode(trustedNode)); @@ -139,16 +140,6 @@ contract StaderOracleTest is Test { assertEq(staderOracle.trustedNodesCount(), 1); assertTrue(staderOracle.isTrustedNode(trustedNode)); - vm.expectRevert(IStaderOracle.NodeNotTrusted.selector); - vm.prank(staderManager); - staderOracle.removeTrustedNode(vm.addr(567)); - - vm.prank(staderManager); - staderOracle.removeTrustedNode(trustedNode); - - assertEq(staderOracle.trustedNodesCount(), 0); - assertFalse(staderOracle.isTrustedNode(trustedNode)); - // lets update trustedNode cooling period vm.expectRevert(UtilLib.CallerNotManager.selector); staderOracle.updateTrustedNodeChangeCoolingPeriod(100); @@ -157,22 +148,36 @@ contract StaderOracleTest is Test { staderOracle.updateTrustedNodeChangeCoolingPeriod(100); vm.expectRevert(IStaderOracle.CooldownNotComplete.selector); - staderOracle.addTrustedNode(vm.addr(78)); + staderOracle.addTrustedNode(vm.addr(77)); - // wait for 100 blocks + // wait for 100 blocks each time to add node + vm.roll(block.number + 100); + staderOracle.addTrustedNode(vm.addr(77)); vm.roll(block.number + 100); staderOracle.addTrustedNode(vm.addr(78)); - assertEq(staderOracle.trustedNodesCount(), 1); + vm.roll(block.number + 100); + staderOracle.addTrustedNode(vm.addr(79)); + assertEq(staderOracle.trustedNodesCount(), 4); + assertTrue(staderOracle.isTrustedNode(vm.addr(77))); assertTrue(staderOracle.isTrustedNode(vm.addr(78))); + assertTrue(staderOracle.isTrustedNode(vm.addr(79))); + + // Tests for removing nodes + vm.expectRevert(IStaderOracle.NodeNotTrusted.selector); + staderOracle.removeTrustedNode(vm.addr(567)); vm.expectRevert(IStaderOracle.CooldownNotComplete.selector); - staderOracle.removeTrustedNode(vm.addr(78)); + staderOracle.removeTrustedNode(vm.addr(77)); // wait for 100 blocks vm.roll(block.number + 100); + staderOracle.removeTrustedNode(vm.addr(77)); + assertEq(staderOracle.trustedNodesCount(), 3); + assertFalse(staderOracle.isTrustedNode(vm.addr(77))); + + vm.roll(block.number + 100); + vm.expectRevert(IStaderOracle.InsufficientTrustedNodes.selector); staderOracle.removeTrustedNode(vm.addr(78)); - assertEq(staderOracle.trustedNodesCount(), 0); - assertFalse(staderOracle.isTrustedNode(vm.addr(78))); vm.stopPrank(); } @@ -189,7 +194,7 @@ contract StaderOracleTest is Test { vm.expectRevert(IStaderOracle.InsufficientTrustedNodes.selector); staderOracle.submitSDPrice(sdPriceData); - assertEq(staderOracle.MIN_TRUSTED_NODES(), 5); + assertEq(staderOracle.MIN_TRUSTED_NODES(), 3); address trustedNode2 = vm.addr(702); address trustedNode3 = vm.addr(703); address trustedNode4 = vm.addr(704); @@ -307,17 +312,12 @@ contract StaderOracleTest is Test { vm.prank(trustedNode3); staderOracle.submitSDPrice(sdPriceData); - sdPriceData.reportingBlockNumber = 5 * 7200; - sdPriceData.sdPriceInETH = 4; - vm.prank(trustedNode4); - staderOracle.submitSDPrice(sdPriceData); - // now consensus is met for reporting block num 5 * 7200 // trustedNode1 manipulated the sd price if other oracles are not wrking properly - // sdPrice submited were [1,6,2,4] => hence median = (2+4)/2 = 3 + // sdPrice submited were [1,6,2] => hence median = 2 (lastSDReportingBlockNumber, lastSDPrice) = staderOracle.lastReportedSDPriceData(); assertEq(lastSDReportingBlockNumber, 5 * 7200); - assertEq(lastSDPrice, 3); + assertEq(lastSDPrice, 2); // trusted node 5 tries to submit at reportable block 5 * 7200 sdPriceData.reportingBlockNumber = 5 * 7200; @@ -330,7 +330,7 @@ contract StaderOracleTest is Test { function test_submitSDPrice_manipulation_not_possible_by_minority_malicious_oracles() public { SDPriceData memory sdPriceData = SDPriceData({ reportingBlockNumber: 1212, sdPriceInETH: 1 }); - assertEq(staderOracle.MIN_TRUSTED_NODES(), 5); + assertEq(staderOracle.MIN_TRUSTED_NODES(), 3); address trustedNode1 = vm.addr(701); address trustedNode2 = vm.addr(702); address trustedNode3 = vm.addr(703); @@ -355,9 +355,6 @@ contract StaderOracleTest is Test { vm.prank(trustedNode2); staderOracle.submitSDPrice(sdPriceData); - vm.prank(trustedNode3); - staderOracle.submitSDPrice(sdPriceData); - // cycle 2 vm.roll(2 * 7200 + 1); sdPriceData.reportingBlockNumber = 2 * 7200; @@ -369,9 +366,6 @@ contract StaderOracleTest is Test { vm.prank(trustedNode2); staderOracle.submitSDPrice(sdPriceData); - vm.prank(trustedNode3); - staderOracle.submitSDPrice(sdPriceData); - // trustedNode4 submits for cycle 1 sdPriceData.reportingBlockNumber = 1 * 7200; sdPriceData.sdPriceInETH = 1; @@ -800,7 +794,7 @@ contract StaderOracleTest is Test { totalETHXSupply: 100 }); - assertEq(staderOracle.MIN_TRUSTED_NODES(), 5); + assertEq(staderOracle.MIN_TRUSTED_NODES(), 3); address trustedNode1 = vm.addr(701); address trustedNode2 = vm.addr(702); address trustedNode3 = vm.addr(703); @@ -1007,7 +1001,7 @@ contract StaderOracleTest is Test { sortedInvalidSignaturePubkeys: invalidSignaturePubkeys }); - assertEq(staderOracle.MIN_TRUSTED_NODES(), 5); + assertEq(staderOracle.MIN_TRUSTED_NODES(), 3); address trustedNode1 = vm.addr(701); address trustedNode2 = vm.addr(702); address trustedNode3 = vm.addr(703); @@ -1082,7 +1076,7 @@ contract StaderOracleTest is Test { sortedPubkeys: sortedPubkeys }); - assertEq(staderOracle.MIN_TRUSTED_NODES(), 5); + assertEq(staderOracle.MIN_TRUSTED_NODES(), 3); address trustedNode1 = vm.addr(701); address trustedNode2 = vm.addr(702); address trustedNode3 = vm.addr(703); @@ -1162,7 +1156,7 @@ contract StaderOracleTest is Test { sortedPubkeys: sortedPubkeys }); - assertEq(staderOracle.MIN_TRUSTED_NODES(), 5); + assertEq(staderOracle.MIN_TRUSTED_NODES(), 3); address trustedNode1 = vm.addr(701); address trustedNode2 = vm.addr(702); address trustedNode3 = vm.addr(703); @@ -1232,7 +1226,7 @@ contract StaderOracleTest is Test { slashedValidatorsCount: 4 }); - assertEq(staderOracle.MIN_TRUSTED_NODES(), 5); + assertEq(staderOracle.MIN_TRUSTED_NODES(), 3); address trustedNode1 = vm.addr(701); address trustedNode2 = vm.addr(702); address trustedNode3 = vm.addr(703);