From 433b8cae1cae1049ff6df22f62b93c7d55c2ccf9 Mon Sep 17 00:00:00 2001 From: julien Date: Wed, 1 Nov 2023 19:00:33 +0100 Subject: [PATCH 01/11] feat(urd): add new values check Signed-off-by: julien --- src/UniversalRewardsDistributor.sol | 11 +++++++++++ src/libraries/ErrorsLib.sol | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/src/UniversalRewardsDistributor.sol b/src/UniversalRewardsDistributor.sol index 7256127..2bf10f7 100644 --- a/src/UniversalRewardsDistributor.sol +++ b/src/UniversalRewardsDistributor.sol @@ -80,9 +80,13 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @param newIpfsHash The optional ipfs hash containing metadata about the root (e.g. the merkle tree itself). /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function submitRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdater { + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); + if (timelock == 0) { _setRoot(newRoot, newIpfsHash); } else { + require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_SET); + pendingRoot = PendingRoot(block.timestamp, newRoot, newIpfsHash); emit EventsLib.RootProposed(newRoot, newIpfsHash); } @@ -139,6 +143,7 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @dev This function can only be called by the owner of the distribution. /// @dev Set to bytes32(0) to remove the root. function setRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyOwner { + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); _setRoot(newRoot, newIpfsHash); } @@ -147,6 +152,8 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @dev This function can only be called by the owner of the distribution. /// @dev If the timelock is reduced, it can only be updated after the timelock has expired. function setTimelock(uint256 newTimelock) external onlyOwner { + require(newTimelock != timelock, ErrorsLib.ALREADY_SET); + if (newTimelock < timelock) { require( pendingRoot.submittedAt == 0 || pendingRoot.submittedAt + timelock <= block.timestamp, @@ -161,6 +168,8 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @param updater The address of the root updater. /// @param active Whether the root updater should be active or not. function setRootUpdater(address updater, bool active) external onlyOwner { + require(isUpdater[updater] != active, ErrorsLib.ALREADY_SET); + isUpdater[updater] = active; emit EventsLib.RootUpdaterSet(updater, active); @@ -180,6 +189,8 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @notice Sets the `owner` of the distribution to `newOwner`. function setOwner(address newOwner) external onlyOwner { + require(newOwner != owner, ErrorsLib.ALREADY_SET); + _setOwner(newOwner); } diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 8e7c820..9a85816 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -26,4 +26,10 @@ library ErrorsLib { /// @notice Thrown when rewards have already been claimed. string internal constant ALREADY_CLAIMED = "already claimed"; + + /// @notice Thrown when the value is already set. + string internal constant ALREADY_SET = "already set"; + + /// @notice Thrown when the submitted pending root is the same as the current one. + string internal constant ROOT_ALREADY_SET = "root already set"; } From 66579b33061956063a21d44c0f544035cecef5af Mon Sep 17 00:00:00 2001 From: julien Date: Wed, 1 Nov 2023 19:00:53 +0100 Subject: [PATCH 02/11] test(urd): test reverts on new values Signed-off-by: julien --- test/UniversalRewardsDistributorTest.sol | 97 +++++++++++++++++++++++- 1 file changed, 96 insertions(+), 1 deletion(-) diff --git a/test/UniversalRewardsDistributorTest.sol b/test/UniversalRewardsDistributorTest.sol index 79f59f7..2c9448a 100644 --- a/test/UniversalRewardsDistributorTest.sol +++ b/test/UniversalRewardsDistributorTest.sol @@ -12,7 +12,6 @@ import {UniversalRewardsDistributor} from "../src/UniversalRewardsDistributor.so import {EventsLib} from "../src/libraries/EventsLib.sol"; import {Merkle} from "../lib/murky/src/Merkle.sol"; - import "../lib/forge-std/src/Test.sol"; contract UniversalRewardsDistributorTest is Test { @@ -153,6 +152,62 @@ contract UniversalRewardsDistributorTest is Test { distributionWithoutTimeLock.submitRoot(DEFAULT_ROOT, DEFAULT_IPFS_HASH); } + function testSubmitRootWithPreviousPendingRootShouldRevert(bytes32 newRoot, bytes32 newIpfsHash) public { + vm.assume(newRoot != distributionWithTimeLock.root() && newIpfsHash != distributionWithTimeLock.ipfsHash()); + + vm.startPrank(owner); + distributionWithTimeLock.submitRoot(newRoot, newIpfsHash); + + vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); + distributionWithTimeLock.submitRoot(newRoot, newIpfsHash); + + vm.stopPrank(); + } + + function testSubmitRootWithCurrentRootAndNoTimelockShouldRevert(bytes32 newRoot, bytes32 newIpfsHash) public { + vm.assume(newRoot != distributionWithTimeLock.root() && newIpfsHash != distributionWithTimeLock.ipfsHash()); + + vm.startPrank(owner); + distributionWithoutTimeLock.submitRoot(newRoot, newIpfsHash); + + vm.expectRevert(bytes(ErrorsLib.ROOT_ALREADY_SET)); + distributionWithoutTimeLock.submitRoot(newRoot, newIpfsHash); + + vm.stopPrank(); + } + + function testSubmitRootWithCurrentRootShouldRevert(bytes32 newRoot, bytes32 newIpfsHash) public { + vm.assume(newRoot != distributionWithTimeLock.root() && newIpfsHash != distributionWithTimeLock.ipfsHash()); + + vm.startPrank(owner); + distributionWithTimeLock.setRoot(newRoot, newIpfsHash); + + vm.expectRevert(bytes(ErrorsLib.ROOT_ALREADY_SET)); + distributionWithTimeLock.submitRoot(newRoot, newIpfsHash); + vm.stopPrank(); + } + + function testSubmitRootTwiceShouldWorkWhenModifyingIpfsHash( + bytes32 newRoot, + bytes32 newIpfsHash, + bytes32 secondIpfsHash + ) public { + vm.assume( + newRoot != distributionWithTimeLock.root() && newIpfsHash != distributionWithTimeLock.ipfsHash() + && secondIpfsHash != newIpfsHash + ); + + vm.startPrank(owner); + distributionWithTimeLock.setRoot(newRoot, newIpfsHash); + + vm.expectEmit(address(distributionWithTimeLock)); + emit EventsLib.RootProposed(newRoot, secondIpfsHash); + distributionWithTimeLock.submitRoot(newRoot, secondIpfsHash); + vm.stopPrank(); + + assertEq(_getPendingRoot(distributionWithTimeLock).ipfsHash, secondIpfsHash); + } + function testSubmitRootWithTimelockAsOwner() public { vm.prank(owner); vm.expectEmit(address(distributionWithTimeLock)); @@ -271,6 +326,8 @@ contract UniversalRewardsDistributorTest is Test { function testSetTimelockShouldChangeTheDistributionTimelock(uint256 newTimelock) public { newTimelock = bound(newTimelock, 0, type(uint256).max); + vm.assume(newTimelock != distributionWithoutTimeLock.timelock()); + vm.prank(owner); vm.expectEmit(address(distributionWithoutTimeLock)); emit EventsLib.TimelockSet(newTimelock); @@ -279,6 +336,19 @@ contract UniversalRewardsDistributorTest is Test { assertEq(distributionWithoutTimeLock.timelock(), newTimelock); } + function testSetTimelockShouldRevertOnSameValue(uint256 newTimelock) public { + newTimelock = bound(newTimelock, 0, type(uint256).max); + + vm.assume(newTimelock != distributionWithoutTimeLock.timelock()); + + vm.prank(owner); + distributionWithoutTimeLock.setTimelock(newTimelock); + + vm.prank(owner); + vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); + distributionWithoutTimeLock.setTimelock(newTimelock); + } + function testSetTimelockShouldIncreaseTheQueueTimestamp( uint256 timeElapsed, uint256 newTimelock, @@ -352,6 +422,8 @@ contract UniversalRewardsDistributorTest is Test { } function testSetRootUpdaterShouldAddOrRemoveRootUpdater(address newUpdater, bool active) public { + vm.assume(distributionWithoutTimeLock.isUpdater(newUpdater) != active); + vm.prank(owner); vm.expectEmit(address(distributionWithoutTimeLock)); emit EventsLib.RootUpdaterSet(newUpdater, active); @@ -360,6 +432,17 @@ contract UniversalRewardsDistributorTest is Test { assertEq(distributionWithoutTimeLock.isUpdater(newUpdater), active); } + function testSetRootUpdaterShouldRevertIfAlreadySet(address newUpdater, bool active) public { + vm.assume(distributionWithoutTimeLock.isUpdater(newUpdater) != active); + + vm.prank(owner); + distributionWithoutTimeLock.setRootUpdater(newUpdater, active); + + vm.prank(owner); + vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); + distributionWithoutTimeLock.setRootUpdater(newUpdater, active); + } + function testSetRootUpdaterShouldRevertIfNotOwner(address caller, bool active) public { vm.assume(caller != owner); @@ -418,6 +501,17 @@ contract UniversalRewardsDistributorTest is Test { distributionWithTimeLock.setOwner(newOwner); } + function testSetOwnerShouldRevertIfAlreadySet(address newOwner) public { + vm.assume(newOwner != owner); + + vm.prank(owner); + distributionWithTimeLock.setOwner(newOwner); + + vm.prank(newOwner); + vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); + distributionWithTimeLock.setOwner(newOwner); + } + function testClaimShouldFollowTheMerkleDistribution(uint256 claimable, uint8 size) public { claimable = bound(claimable, 1 ether, 1000 ether); uint256 boundedSize = bound(size, 2, MAX_RECEIVERS); @@ -452,6 +546,7 @@ contract UniversalRewardsDistributorTest is Test { } function testClaimShouldReturnsTheAmountClaimed(uint256 claimable) public { + vm.assume(claimable > 0); claimable = bound(claimable, 1 ether, 1000 ether); (bytes32[] memory data, bytes32 root) = _setupRewards(claimable, 2); From f723c1ef8601fd787f5c396314615c5a83292103 Mon Sep 17 00:00:00 2001 From: Julien THOMAS <61523188+julien-devatom@users.noreply.github.com> Date: Thu, 2 Nov 2023 08:43:39 +0100 Subject: [PATCH 03/11] fix(tests): harmonize test names Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Signed-off-by: Julien THOMAS <61523188+julien-devatom@users.noreply.github.com> --- test/UniversalRewardsDistributorTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/UniversalRewardsDistributorTest.sol b/test/UniversalRewardsDistributorTest.sol index 2c9448a..9ee3968 100644 --- a/test/UniversalRewardsDistributorTest.sol +++ b/test/UniversalRewardsDistributorTest.sol @@ -336,7 +336,7 @@ contract UniversalRewardsDistributorTest is Test { assertEq(distributionWithoutTimeLock.timelock(), newTimelock); } - function testSetTimelockShouldRevertOnSameValue(uint256 newTimelock) public { + function testSetTimelockShouldRevertIfSameValue(uint256 newTimelock) public { newTimelock = bound(newTimelock, 0, type(uint256).max); vm.assume(newTimelock != distributionWithoutTimeLock.timelock()); From ad65219cae88d78f444f0f89390a0c8e2727f366 Mon Sep 17 00:00:00 2001 From: julien Date: Thu, 2 Nov 2023 08:46:21 +0100 Subject: [PATCH 04/11] refactor(urd): move root set check to `_setRoot` --- src/UniversalRewardsDistributor.sol | 6 +++--- src/libraries/ErrorsLib.sol | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/UniversalRewardsDistributor.sol b/src/UniversalRewardsDistributor.sol index 2bf10f7..54227ce 100644 --- a/src/UniversalRewardsDistributor.sol +++ b/src/UniversalRewardsDistributor.sol @@ -80,12 +80,11 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @param newIpfsHash The optional ipfs hash containing metadata about the root (e.g. the merkle tree itself). /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function submitRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdater { - require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); - if (timelock == 0) { _setRoot(newRoot, newIpfsHash); } else { require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_SET); + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); pendingRoot = PendingRoot(block.timestamp, newRoot, newIpfsHash); emit EventsLib.RootProposed(newRoot, newIpfsHash); @@ -143,7 +142,6 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @dev This function can only be called by the owner of the distribution. /// @dev Set to bytes32(0) to remove the root. function setRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyOwner { - require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); _setRoot(newRoot, newIpfsHash); } @@ -200,6 +198,8 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor { /// @dev Deletes the pending root. /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function _setRoot(bytes32 newRoot, bytes32 newIpfsHash) internal { + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); + root = newRoot; ipfsHash = newIpfsHash; diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 9a85816..24ca0f4 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -30,6 +30,6 @@ library ErrorsLib { /// @notice Thrown when the value is already set. string internal constant ALREADY_SET = "already set"; - /// @notice Thrown when the submitted pending root is the same as the current one. + /// @notice Thrown when the submitted root (pending or not) is the same as the current one. string internal constant ROOT_ALREADY_SET = "root already set"; } From bbe5c7321711dedb81c00c7689d92d0195c6ba5d Mon Sep 17 00:00:00 2001 From: julien Date: Mon, 6 Nov 2023 09:50:02 +0100 Subject: [PATCH 05/11] fix(test): stop using `_getPendingRoot` --- test/UniversalRewardsDistributorTest.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/UniversalRewardsDistributorTest.sol b/test/UniversalRewardsDistributorTest.sol index 733702d..182d43a 100644 --- a/test/UniversalRewardsDistributorTest.sol +++ b/test/UniversalRewardsDistributorTest.sol @@ -214,7 +214,7 @@ contract UniversalRewardsDistributorTest is Test { distributionWithTimeLock.submitRoot(newRoot, secondIpfsHash); vm.stopPrank(); - assertEq(_getPendingRoot(distributionWithTimeLock).ipfsHash, secondIpfsHash); + assertEq(distributionWithTimeLock.pendingRoot().ipfsHash, secondIpfsHash); } function testSubmitRootWithTimelockAsOwner() public { From 579c3f4c257b55043fa2ff9d654ec19e01343d24 Mon Sep 17 00:00:00 2001 From: julien Date: Mon, 6 Nov 2023 10:29:08 +0100 Subject: [PATCH 06/11] fix(events): let the constructor emitting same value --- src/UniversalRewardsDistributor.sol | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/UniversalRewardsDistributor.sol b/src/UniversalRewardsDistributor.sol index 2dbb592..41cddbd 100644 --- a/src/UniversalRewardsDistributor.sol +++ b/src/UniversalRewardsDistributor.sol @@ -78,11 +78,12 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @param newIpfsHash The optional ipfs hash containing metadata about the root (e.g. the merkle tree itself). /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function submitRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdaterRole { + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); + if (timelock == 0) { _setRoot(newRoot, newIpfsHash); } else { require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_SET); - require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); pendingRoot = PendingRoot(block.timestamp + timelock, newRoot, newIpfsHash); emit EventsLib.RootProposed(newRoot, newIpfsHash); @@ -135,6 +136,8 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @dev This function can only be called by the owner of the distribution. /// @dev Set to bytes32(0) to remove the root. function setRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyOwner { + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); + _setRoot(newRoot, newIpfsHash); } @@ -143,6 +146,8 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @dev This function can only be called by the owner of the distribution. /// @dev The timelock modification are not applicable to the pending values. function setTimelock(uint256 newTimelock) external onlyOwner { + require(newTimelock != timelock, ErrorsLib.ALREADY_SET); + _setTimelock(newTimelock); } @@ -182,7 +187,6 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @dev Deletes the pending root. /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function _setRoot(bytes32 newRoot, bytes32 newIpfsHash) internal { - require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); root = newRoot; ipfsHash = newIpfsHash; From ecea33cef6bff4e474e42d3ce9a55f4b1322ba7c Mon Sep 17 00:00:00 2001 From: julien Date: Mon, 6 Nov 2023 10:31:41 +0100 Subject: [PATCH 07/11] fix(events): emit `ALREADY_PENDING` if the value is the pending one --- src/UniversalRewardsDistributor.sol | 6 +++--- src/libraries/ErrorsLib.sol | 2 +- test/UniversalRewardsDistributorTest.sol | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/UniversalRewardsDistributor.sol b/src/UniversalRewardsDistributor.sol index 41cddbd..ff37f80 100644 --- a/src/UniversalRewardsDistributor.sol +++ b/src/UniversalRewardsDistributor.sol @@ -78,12 +78,12 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @param newIpfsHash The optional ipfs hash containing metadata about the root (e.g. the merkle tree itself). /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function submitRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdaterRole { - require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ALREADY_SET); if (timelock == 0) { _setRoot(newRoot, newIpfsHash); } else { - require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_SET); + require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_PENDING); pendingRoot = PendingRoot(block.timestamp + timelock, newRoot, newIpfsHash); emit EventsLib.RootProposed(newRoot, newIpfsHash); @@ -136,7 +136,7 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @dev This function can only be called by the owner of the distribution. /// @dev Set to bytes32(0) to remove the root. function setRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyOwner { - require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ROOT_ALREADY_SET); + require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ALREADY_SET); _setRoot(newRoot, newIpfsHash); } diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 9c6731b..8bc1c20 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -31,5 +31,5 @@ library ErrorsLib { string internal constant ALREADY_SET = "already set"; /// @notice Thrown when the submitted root (pending or not) is the same as the current one. - string internal constant ROOT_ALREADY_SET = "root already set"; + string internal constant ALREADY_PENDING = "already pending"; } diff --git a/test/UniversalRewardsDistributorTest.sol b/test/UniversalRewardsDistributorTest.sol index 182d43a..c758999 100644 --- a/test/UniversalRewardsDistributorTest.sol +++ b/test/UniversalRewardsDistributorTest.sol @@ -167,7 +167,7 @@ contract UniversalRewardsDistributorTest is Test { vm.startPrank(owner); distributionWithTimeLock.submitRoot(newRoot, newIpfsHash); - vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); + vm.expectRevert(bytes(ErrorsLib.ALREADY_PENDING)); distributionWithTimeLock.submitRoot(newRoot, newIpfsHash); vm.stopPrank(); @@ -179,7 +179,7 @@ contract UniversalRewardsDistributorTest is Test { vm.startPrank(owner); distributionWithoutTimeLock.submitRoot(newRoot, newIpfsHash); - vm.expectRevert(bytes(ErrorsLib.ROOT_ALREADY_SET)); + vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); distributionWithoutTimeLock.submitRoot(newRoot, newIpfsHash); vm.stopPrank(); @@ -191,7 +191,7 @@ contract UniversalRewardsDistributorTest is Test { vm.startPrank(owner); distributionWithTimeLock.setRoot(newRoot, newIpfsHash); - vm.expectRevert(bytes(ErrorsLib.ROOT_ALREADY_SET)); + vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); distributionWithTimeLock.submitRoot(newRoot, newIpfsHash); vm.stopPrank(); } From a9d266932f1ed3c4f30b9c8a8cbf03d40ef28adb Mon Sep 17 00:00:00 2001 From: julien Date: Mon, 6 Nov 2023 13:31:16 +0100 Subject: [PATCH 08/11] fix(errors): lint & natspecs --- src/UniversalRewardsDistributor.sol | 1 - src/libraries/ErrorsLib.sol | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/UniversalRewardsDistributor.sol b/src/UniversalRewardsDistributor.sol index ff37f80..bc23b5e 100644 --- a/src/UniversalRewardsDistributor.sol +++ b/src/UniversalRewardsDistributor.sol @@ -187,7 +187,6 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @dev Deletes the pending root. /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function _setRoot(bytes32 newRoot, bytes32 newIpfsHash) internal { - root = newRoot; ipfsHash = newIpfsHash; diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index 8bc1c20..df23b93 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -30,6 +30,6 @@ library ErrorsLib { /// @notice Thrown when the value is already set. string internal constant ALREADY_SET = "already set"; - /// @notice Thrown when the submitted root (pending or not) is the same as the current one. + /// @notice Thrown when the submitted value is already pending. string internal constant ALREADY_PENDING = "already pending"; } From eb8e5f7c9dac45f4a7f310c5ab3367e61c498aa2 Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Mon, 6 Nov 2023 16:58:38 +0100 Subject: [PATCH 09/11] refactor: streamline submitRoot and setRoot --- src/UniversalRewardsDistributor.sol | 16 ++-- src/libraries/ErrorsLib.sol | 3 + test/UniversalRewardsDistributorTest.sol | 104 ++++++++++------------- 3 files changed, 53 insertions(+), 70 deletions(-) diff --git a/src/UniversalRewardsDistributor.sol b/src/UniversalRewardsDistributor.sol index bc23b5e..15a9b1e 100644 --- a/src/UniversalRewardsDistributor.sol +++ b/src/UniversalRewardsDistributor.sol @@ -78,16 +78,11 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @param newIpfsHash The optional ipfs hash containing metadata about the root (e.g. the merkle tree itself). /// @dev Warning: The `newIpfsHash` might not correspond to the `newRoot`. function submitRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdaterRole { - require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ALREADY_SET); + require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_PENDING); - if (timelock == 0) { - _setRoot(newRoot, newIpfsHash); - } else { - require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_PENDING); + pendingRoot = PendingRoot(block.timestamp + timelock, newRoot, newIpfsHash); - pendingRoot = PendingRoot(block.timestamp + timelock, newRoot, newIpfsHash); - emit EventsLib.RootProposed(newRoot, newIpfsHash); - } + emit EventsLib.RootProposed(newRoot, newIpfsHash); } /// @notice Accepts and sets the current pending merkle root. @@ -133,10 +128,11 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @notice Forces update the root of a given distribution (bypassing the timelock). /// @param newRoot The new merkle root. /// @param newIpfsHash The optional ipfs hash containing metadata about the root (e.g. the merkle tree itself). - /// @dev This function can only be called by the owner of the distribution. + /// @dev This function can only be called by the owner of the distribution or by updaters if there are no timelock. /// @dev Set to bytes32(0) to remove the root. - function setRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyOwner { + function setRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdaterRole { require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ALREADY_SET); + require(timelock == 0 || msg.sender == owner, ErrorsLib.UNAUTHORIZED_ROOT_CHANGE); _setRoot(newRoot, newIpfsHash); } diff --git a/src/libraries/ErrorsLib.sol b/src/libraries/ErrorsLib.sol index df23b93..cfd195d 100644 --- a/src/libraries/ErrorsLib.sol +++ b/src/libraries/ErrorsLib.sol @@ -12,6 +12,9 @@ library ErrorsLib { /// @notice Thrown when the caller is not the owner. string internal constant NOT_OWNER = "caller is not the owner"; + /// @notice Thrown when the caller trying to change the root under timelock is not the owner. + string internal constant UNAUTHORIZED_ROOT_CHANGE = "unauthorized to change the root"; + /// @notice Thrown when there is not pending root. string internal constant NO_PENDING_ROOT = "no pending root"; diff --git a/test/UniversalRewardsDistributorTest.sol b/test/UniversalRewardsDistributorTest.sol index c758999..921a68d 100644 --- a/test/UniversalRewardsDistributorTest.sol +++ b/test/UniversalRewardsDistributorTest.sol @@ -125,34 +125,6 @@ contract UniversalRewardsDistributorTest is Test { ); } - function testSubmitRootWithoutTimelockAsOwner() public { - vm.prank(owner); - vm.expectEmit(address(distributionWithoutTimeLock)); - emit EventsLib.RootSet(DEFAULT_ROOT, DEFAULT_IPFS_HASH); - distributionWithoutTimeLock.submitRoot(DEFAULT_ROOT, DEFAULT_IPFS_HASH); - - assertEq(distributionWithoutTimeLock.root(), DEFAULT_ROOT); - assertEq(distributionWithoutTimeLock.ipfsHash(), DEFAULT_IPFS_HASH); - PendingRoot memory pendingRoot = distributionWithoutTimeLock.pendingRoot(); - assertEq(pendingRoot.root, bytes32(0)); - assertEq(pendingRoot.validAt, 0); - assertEq(pendingRoot.ipfsHash, bytes32(0)); - } - - function testSubmitRootWithoutTimelockAsUpdater() public { - vm.prank(updater); - vm.expectEmit(address(distributionWithoutTimeLock)); - emit EventsLib.RootSet(DEFAULT_ROOT, DEFAULT_IPFS_HASH); - distributionWithoutTimeLock.submitRoot(DEFAULT_ROOT, DEFAULT_IPFS_HASH); - - assertEq(distributionWithoutTimeLock.root(), DEFAULT_ROOT); - assertEq(distributionWithoutTimeLock.ipfsHash(), DEFAULT_IPFS_HASH); - PendingRoot memory pendingRoot = distributionWithoutTimeLock.pendingRoot(); - assertEq(pendingRoot.root, bytes32(0)); - assertEq(pendingRoot.validAt, 0); - assertEq(pendingRoot.ipfsHash, bytes32(0)); - } - function testSubmitRootWithoutTimelockAsRandomCallerShouldRevert(address randomCaller) public { vm.assume(!distributionWithoutTimeLock.isUpdater(randomCaller) && randomCaller != owner); @@ -173,29 +145,6 @@ contract UniversalRewardsDistributorTest is Test { vm.stopPrank(); } - function testSubmitRootWithCurrentRootAndNoTimelockShouldRevert(bytes32 newRoot, bytes32 newIpfsHash) public { - vm.assume(newRoot != distributionWithTimeLock.root() && newIpfsHash != distributionWithTimeLock.ipfsHash()); - - vm.startPrank(owner); - distributionWithoutTimeLock.submitRoot(newRoot, newIpfsHash); - - vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); - distributionWithoutTimeLock.submitRoot(newRoot, newIpfsHash); - - vm.stopPrank(); - } - - function testSubmitRootWithCurrentRootShouldRevert(bytes32 newRoot, bytes32 newIpfsHash) public { - vm.assume(newRoot != distributionWithTimeLock.root() && newIpfsHash != distributionWithTimeLock.ipfsHash()); - - vm.startPrank(owner); - distributionWithTimeLock.setRoot(newRoot, newIpfsHash); - - vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); - distributionWithTimeLock.submitRoot(newRoot, newIpfsHash); - vm.stopPrank(); - } - function testSubmitRootTwiceShouldWorkWhenModifyingIpfsHash( bytes32 newRoot, bytes32 newIpfsHash, @@ -294,12 +243,32 @@ contract UniversalRewardsDistributorTest is Test { distributionWithTimeLock.acceptRoot(); } - function testSetRootShouldRevertIfNotOwner(bytes32 newRoot, address randomCaller) public { + function testSetRootWithoutTimelockAsRandomCallerShouldRevert(address randomCaller) public { + vm.assume(!distributionWithoutTimeLock.isUpdater(randomCaller) && randomCaller != owner); + + vm.prank(randomCaller); + vm.expectRevert(bytes(ErrorsLib.NOT_UPDATER_ROLE)); + distributionWithoutTimeLock.setRoot(DEFAULT_ROOT, DEFAULT_IPFS_HASH); + } + + function testSetRootWithTimelockShouldRevertIfNotOwner(bytes32 newRoot, address randomCaller) public { vm.assume(randomCaller != owner); vm.prank(randomCaller); - vm.expectRevert(bytes(ErrorsLib.NOT_OWNER)); - distributionWithoutTimeLock.setRoot(newRoot, DEFAULT_IPFS_HASH); + vm.expectRevert(); + distributionWithTimeLock.setRoot(newRoot, DEFAULT_IPFS_HASH); + } + + function testSetRootWithPreviousRootShouldRevert(bytes32 newRoot, bytes32 newIpfsHash) public { + vm.assume(newRoot != distributionWithTimeLock.root() && newIpfsHash != distributionWithTimeLock.ipfsHash()); + + vm.startPrank(owner); + distributionWithTimeLock.setRoot(newRoot, newIpfsHash); + + vm.expectRevert(bytes(ErrorsLib.ALREADY_SET)); + distributionWithTimeLock.setRoot(newRoot, newIpfsHash); + + vm.stopPrank(); } function testSetRootShouldUpdateTheCurrentRoot(bytes32 newRoot, bytes32 newIpfsHash) public { @@ -318,6 +287,21 @@ contract UniversalRewardsDistributorTest is Test { assertEq(pendingRoot.validAt, 0); } + function testSetRootWithoutTimelockAsUpdater() public { + vm.prank(updater); + vm.expectEmit(address(distributionWithoutTimeLock)); + emit EventsLib.RootSet(DEFAULT_ROOT, DEFAULT_IPFS_HASH); + distributionWithoutTimeLock.setRoot(DEFAULT_ROOT, DEFAULT_IPFS_HASH); + + assertEq(distributionWithoutTimeLock.root(), DEFAULT_ROOT); + assertEq(distributionWithoutTimeLock.ipfsHash(), DEFAULT_IPFS_HASH); + PendingRoot memory pendingRoot = distributionWithoutTimeLock.pendingRoot(); + assertEq(pendingRoot.root, bytes32(0)); + assertEq(pendingRoot.validAt, 0); + assertEq(pendingRoot.ipfsHash, bytes32(0)); + } + + function testSetRootShouldRemovePendingRoot(bytes32 newRoot, address randomCaller) public { vm.assume(newRoot != DEFAULT_ROOT && randomCaller != owner); @@ -525,7 +509,7 @@ contract UniversalRewardsDistributorTest is Test { (bytes32[] memory data, bytes32 root) = _setupRewards(claimable, boundedSize); vm.prank(owner); - distributionWithoutTimeLock.submitRoot(root, DEFAULT_IPFS_HASH); + distributionWithoutTimeLock.setRoot(root, DEFAULT_IPFS_HASH); assertEq(distributionWithoutTimeLock.root(), root); @@ -538,7 +522,7 @@ contract UniversalRewardsDistributorTest is Test { (bytes32[] memory data, bytes32 root) = _setupRewards(claimable, 2); vm.prank(owner); - distributionWithoutTimeLock.submitRoot(root, DEFAULT_IPFS_HASH); + distributionWithoutTimeLock.setRoot(root, DEFAULT_IPFS_HASH); assertEq(distributionWithoutTimeLock.root(), root); bytes32[] memory proof1 = merkle.getProof(data, 0); @@ -551,14 +535,14 @@ contract UniversalRewardsDistributorTest is Test { distributionWithoutTimeLock.claim(vm.addr(1), address(token1), claimable, proof1); } - function testClaimShouldReturnsTheAmountClaimed(uint256 claimable) public { + function testClaimShouldReturnTheAmountClaimed(uint256 claimable) public { vm.assume(claimable > 0); claimable = bound(claimable, 1 ether, 1000 ether); (bytes32[] memory data, bytes32 root) = _setupRewards(claimable, 2); vm.prank(owner); - distributionWithoutTimeLock.submitRoot(root, DEFAULT_IPFS_HASH); + distributionWithoutTimeLock.setRoot(root, DEFAULT_IPFS_HASH); assertEq(distributionWithoutTimeLock.root(), root); bytes32[] memory proof1 = merkle.getProof(data, 0); @@ -572,7 +556,7 @@ contract UniversalRewardsDistributorTest is Test { (bytes32[] memory data2, bytes32 root2) = _setupRewards(claimable * 2, 2); vm.prank(owner); - distributionWithoutTimeLock.submitRoot(root2, DEFAULT_IPFS_HASH); + distributionWithoutTimeLock.setRoot(root2, DEFAULT_IPFS_HASH); assertEq(distributionWithoutTimeLock.root(), root2); bytes32[] memory proof2 = merkle.getProof(data2, 0); @@ -602,7 +586,7 @@ contract UniversalRewardsDistributorTest is Test { vm.assume(root != invalidRoot); vm.prank(owner); - distributionWithoutTimeLock.submitRoot(invalidRoot, DEFAULT_IPFS_HASH); + distributionWithoutTimeLock.setRoot(invalidRoot, DEFAULT_IPFS_HASH); bytes32[] memory proof1 = merkle.getProof(data, 0); From 7d54bc4cdeaecd14ba144179769d50633116ad09 Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Mon, 6 Nov 2023 17:11:23 +0100 Subject: [PATCH 10/11] fix: linting --- test/UniversalRewardsDistributorTest.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/UniversalRewardsDistributorTest.sol b/test/UniversalRewardsDistributorTest.sol index 921a68d..02ad82f 100644 --- a/test/UniversalRewardsDistributorTest.sol +++ b/test/UniversalRewardsDistributorTest.sol @@ -301,7 +301,6 @@ contract UniversalRewardsDistributorTest is Test { assertEq(pendingRoot.ipfsHash, bytes32(0)); } - function testSetRootShouldRemovePendingRoot(bytes32 newRoot, address randomCaller) public { vm.assume(newRoot != DEFAULT_ROOT && randomCaller != owner); From 5bcef7fa528cac12aae39c74776710bc4505a7d1 Mon Sep 17 00:00:00 2001 From: Quentin Garchery Date: Tue, 7 Nov 2023 13:43:37 +0100 Subject: [PATCH 11/11] docs: typo in setRoot comment Co-authored-by: Merlin Egalite <44097430+MerlinEgalite@users.noreply.github.com> Signed-off-by: Quentin Garchery --- src/UniversalRewardsDistributor.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/UniversalRewardsDistributor.sol b/src/UniversalRewardsDistributor.sol index 15a9b1e..20e6ec6 100644 --- a/src/UniversalRewardsDistributor.sol +++ b/src/UniversalRewardsDistributor.sol @@ -128,7 +128,7 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping /// @notice Forces update the root of a given distribution (bypassing the timelock). /// @param newRoot The new merkle root. /// @param newIpfsHash The optional ipfs hash containing metadata about the root (e.g. the merkle tree itself). - /// @dev This function can only be called by the owner of the distribution or by updaters if there are no timelock. + /// @dev This function can only be called by the owner of the distribution or by updaters if there is no timelock. /// @dev Set to bytes32(0) to remove the root. function setRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdaterRole { require(newRoot != root || newIpfsHash != ipfsHash, ErrorsLib.ALREADY_SET);