Merge pull request #98 from morpho-org/cantina-review

Cantina review

README.md

@@ -1,50 +1,106 @@
-# Universal Permissionless Rewards Distributor
+# Universal Rewards Distributor
 
-A universal permissionless rewards distributor written in Solidity. It allows the distribution of any reward token (different reward tokens are possible simultaneously) based on a Merkle tree distribution, and using ERC20 allowance of a treasury.
+The Universal Rewards Distributor (URD), is a smart contract allowing the distribution of multiple ERC20 tokens from a single offchain computed Merkle tree.
 
-The singleton contract allows any treasury to distribute rewards to any address, based on a Merkle tree. The Merkle root is stored in the contract. The Merkle root can be updated by whitelisted users. The distribution owner can freeze, force update, or suggest a new treasury. The treasury must accept the role and set an allowance to distribute rewards.
+Each URD contract has an owner and a group of updaters (chosen by the owner). Values submitted by updaters are timelocked and can be revoked by the owner or overriden by another updater. However, this timelock can be set to 0 if the URD owner does not need it.
 
-Based on [Morpho's rewards distributor](https://github.com/morpho-dao/morpho-v1/blob/main/src/common/rewards-distribution/RewardsDistributor.sol), itself based on [Euler's rewards distributor](https://github.com/euler-xyz/euler-contracts/blob/master/contracts/mining/EulDistributor.sol).
+## Use Case Example
 
-Tests are using [Murky](https://github.com/dmfxyz/murky), to generate Merkle trees in Solidity.
+Assume the Owner is a DAO with a periodic rewards mechanism. Each month, a [Gelato](https://www.gelato.network/) bot runs a script to create a Merkle tree that distributes TokenA and TokenB.
 
-## Usage
+During the setup, the DAO configures the timelock based on the risk of updater corruption, let's say 3 days, and adds a Gelato bot as an updater. If the DAO already has a distribution at the time of the URD deployment, it can define an initial root, or use an empty root if not.
 
-Merkle trees should be generated with [Openzeppelin library](https://github.com/OpenZeppelin/merkle-tree).
-It will ensure that trees will be secure for on-chain verification.
+Each month, the Gelato bot proposes a new root. For 3 days, the DAO has the opportunity to run checks on this root. After these 3 days, if the DAO did not revoke the root, anyone can accept this value.
 
-## Installation
+The DAO must transfer the correct amount of tokens to the URD to allow all claimants to claim their rewards. If the DAO does not provide enough funds, the claim function will fail with the message "not enough funds".
 
-Download foundry:
-```bash
-curl -L https://foundry.paradigm.xyz | bash
-```
+## Attaching an IPFS Hash
 
-Install it:
-```bash
-foundryup
-```
+- Using a Merkle tree delegates all root computation offchain, leaving no information about the tree onchain (except for the root). This makes it challenging for an integrator (or a claimer) to understand the Merkle tree or to know which proof to use. To address this, each root can be linked to an IPFS hash, which can link to any data. We recommend that all users follow the same format for the IPFS hash to facilitate integration. The suggested format is as follows:
 
-Install dependencies:
-```bash
-git submodule update --init --recursive
+```json
+{
+  "id": "A string id of the Merkle tree, can be random",
+  "metadata": {
+    "info": "a key value mapping allowing you to add information"
+  },
+  "root": "The merkle root of the tree",
+  "tree": [
+    {
+      "account": "The address of the claimer",
+      "reward": "The address of the reward token",
+      "claimable": "The claimable amount as a big number",
+      "proof": ["0x1...", "0x2...", "...", "0xN..."]
+    }
+  ]
+}
 ```
 
-Now you can run tests, using forge:
-```bash
-forge test
-```
+- We recommend not including spaces or new lines when uploading to IPFS, to ensure a consistent method of uploading your JSON file. 
+- We also recommend sorting the tree by the account address to ensure a consistent order.
+
+## Owner Specifications
+
+- The URD is an owner-managed contract, meaning the owner has full control over its distribution. Specifically, the owner can bypass all timelocked functions, modify the timelock, add or remove updaters, and revoke the pending value at their discretion.
+- If the owner is set to the zero address, the contract becomes ownerless. In this scenario, only updaters can submit a root. Furthermore, the time-lock value becomes unchangeable, and pending values cannot be removed.
+- If there are neither owner nor updaters, the URD becomes trustless. This is useful for creating a one-time distribution that must remain unchanged. This can be accomplished at the contract's creation by providing only a Merkle root and an optional IPFS hash. After this, the contract's sole functionality is to claim rewards.
+- It is possible to create a URD with no root, owner, or updaters. While this might seem pointless, it is the URD creator's responsibility to configure the URD correctly.
+
+## Updaters Specifications
+
+- Multiple updaters can be defined for a single URD. The owner can add or remove updaters at any time, instantly, without a timelock.
+- All updaters share a single pending value. This means they can override the pending value (if any) at any time.
+- If a pending value is not used as the main root, any submissions by updaters will override the pending value.
+
+Having multiple updaters can lead to situations where the pending values are subject to multiple concurrent propositions. The owner must manage this scenario to maintain the URD's functionality. We recommend not having different reward distribution strategies in a single URD. All updaters should agree on a distribution mechanism and a root. The use case for having multiple updaters is to enhance resilience and security.
+
+## Considerations for Merkle Tree
+
+- The claimable amount for a given user must always exceed the amount provided in the previous Merkle tree. If a claimer has claimed an amount higher than the `claimable` amount in the Merkle tree (if claimed from a previous root), the claim will revert with a "root misconfigured" error.
+- TODO: Define the list of invariants for a new root.
+- We recommend merging all the {reward, user} pairs into a single leaf. If you wish to have two different leaves for one {reward, user} pair, the user will be able to claim the larger amount from the two leaves, not the sum of the two.
+- Merkle trees can be generated with [Openzeppelin library](https://github.com/OpenZeppelin/merkle-tree).
+- The leaves of the merkle tree have to be hashed twice. It is natively supported by the Openzeppelin library.
+- The URD supports empty root. This means that, at any time, updaters or owner can submit a 0 hash root. It can be used to deprecate the URD.
+
+## Claim Rewards
+
+- The arguments for the claim function must be provided off-chain. If an IPFS hash is given, the IPFS content likely includes the parameters.
+- Anybody can make a claim on behalf of someone else. This means you can claim for multiple users in one transaction by using a multicall.
+- An update to the root can increase the claimable amount and modify the proof, even if the claimable amount remains unchanged.
+- A call to the claim function will claim only one token. If you wish to claim multiple tokens, use a multicall.
+
+## The factory
+
+You can create a URD using a factory. This factory simplifies the indexing of the URD offchain and validates any URDs created through it. While its use is optional, it offers a convenient alternative to directly deploying a URD by submitting the bytecode.
+
+## Skim non claimed rewards
+A rewards program can have a deadline for users to claim tokens. 
+After this deadline, the owner can skim the rewards that were not claimed. 
+To do so, the owner has to create a Merkle tree that is sending the rewards to the owner. 
+Then, the owner can submit this Merkle tree to the URD. The URD will then allow the owner to claim the rewards that were not claimed by the users.
 
 ## Limitations
 
 ### The pending root is not a queue
 
-The pending root does not have a queue mechanism. Therefore, pushing a root to the pending root as a root updater in a distribution with a timelock will essentially erase the previous root. This means that a compromised root updater can always suggest a root and reset the timelock of the pending root.
+The pending root does not have a queue mechanism. As a result, when a root updater pushes a root to the pending root in a distribution with a timelock, it effectively erases the previous root. This implies that a compromised root updater can continuously suggest a root, resetting the timelock of the pending root.
+
+Furthermore, if the pending root is ready to be accepted but a root updater suggests a new root simultaneously, the pending root is erased and the timelock restarts. This situation can lead to endless loops of the pending root, especially if an automation mechanism suggests a root at intervals shorter than the timelock.
 
-Additionally, if the pending root is ready to be accepted but a root updater suggests a new root at the same time, the pending root will be erased and the timelock will restart.
+This behavior is acknowledged and should be considered when designing a strategy using the URD. Ensure the epoch interval for updating the root is longer than the timelock, and define these parameters accordingly.
 
-This can lead to infinite loops of pending root if an automation mechanism suggests a root at an interval smaller than the timelock.
+An alternative solution is to build a queue mechanism on top of the URD with a 0 timelock distribution. Here, the root updater could be a queue designed as the [Delay Modifier of Zodiac](https://github.com/gnosis/zodiac-modifier-delay/blob/36f56fd2e7a4aeb128971c5567fb8dffb6c6a21b/contracts/Delay.sol).
+
+
+## Development
+
+Running tests requires forge from [Foundry](https://book.getfoundry.sh/getting-started/installation).
+
+```bash
+forge test
+```
 
-This behavior is acknowledged. When designing a strategy on top of the URD, you must ensure that the epoch interval at which you update the root is longer than the timelock. Define these parameters accordingly.
+## Licence
 
-Additionally, you can build a queue mechanism on top of the URD with a 0 timelock distribution, where the root updater is a queue designed as the [Delay Modifier of Zodiac](https://github.com/gnosis/zodiac-modifier-delay/blob/36f56fd2e7a4aeb128971c5567fb8dffb6c6a21b/contracts/Delay.sol).
+The URD is licensed under the [AGPLv2](https://www.gnu.org/licenses/old-licenses/gpl-2.0.html) or later.

package.json

@@ -26,10 +26,10 @@
     "clean": "forge clean"
   },
   "devDependencies": {
-    "@commitlint/cli": "^17.7.1",
-    "@commitlint/config-conventional": "^17.7.0",
+    "@commitlint/cli": "^18.2.0",
+    "@commitlint/config-conventional": "^18.1.0",
     "husky": "^8.0.3",
-    "lint-staged": "^14.0.1"
+    "lint-staged": "^15.0.2"
   },
   "lint-staged": {
     "*.sol": "forge fmt"

src/UniversalRewardsDistributor.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-pragma solidity 0.8.21;
+pragma solidity 0.8.19;
 
-import {PendingRoot, IUniversalRewardsDistributor} from "./interfaces/IUniversalRewardsDistributor.sol";
+import {PendingRoot, IUniversalRewardsDistributorStaticTyping} from "./interfaces/IUniversalRewardsDistributor.sol";
 
 import {ErrorsLib} from "./libraries/ErrorsLib.sol";
 import {EventsLib} from "./libraries/EventsLib.sol";
@@ -15,7 +15,7 @@ import {MerkleProof} from "../lib/openzeppelin-contracts/contracts/utils/cryptog
 /// @notice This contract enables the distribution of various reward tokens to multiple accounts using different
 /// permissionless Merkle trees. It is largely inspired by Morpho's current rewards distributor:
 /// https://github.com/morpho-dao/morpho-v1/blob/main/src/common/rewards-distribution/RewardsDistributor.sol
-contract UniversalRewardsDistributor is IUniversalRewardsDistributor {
+contract UniversalRewardsDistributor is IUniversalRewardsDistributorStaticTyping {
     using SafeTransferLib for ERC20;
 
     /* STORAGE */
@@ -47,13 +47,13 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor {
 
     /// @notice Reverts if the caller is not the owner.
     modifier onlyOwner() {
-        require(msg.sender == owner, ErrorsLib.CALLER_NOT_OWNER);
+        require(msg.sender == owner, ErrorsLib.NOT_OWNER);
         _;
     }
 
-    /// @notice Reverts if the caller is not the owner nor an updater.
-    modifier onlyUpdater() {
-        require(isUpdater[msg.sender] || msg.sender == owner, ErrorsLib.CALLER_NOT_OWNER_OR_UPDATER);
+    /// @notice Reverts if the caller has not the updater role.
+    modifier onlyUpdaterRole() {
+        require(isUpdater[msg.sender] || msg.sender == owner, ErrorsLib.NOT_UPDATER_ROLE);
         _;
     }
 
@@ -67,10 +67,8 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor {
     /// @dev Warning: The `initialIpfsHash` might not correspond to the `initialRoot`.
     constructor(address initialOwner, uint256 initialTimelock, bytes32 initialRoot, bytes32 initialIpfsHash) {
         _setOwner(initialOwner);
-
-        if (initialTimelock > 0) _setTimelock(initialTimelock);
-
-        if (initialRoot != bytes32(0)) _setRoot(initialRoot, initialIpfsHash);
+        _setTimelock(initialTimelock);
+        _setRoot(initialRoot, initialIpfsHash);
     }
 
     /* EXTERNAL */
@@ -79,28 +77,32 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor {
     /// @param newRoot The new merkle root.
     /// @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 {
-        if (timelock == 0) {
-            _setRoot(newRoot, newIpfsHash);
-        } else {
-            pendingRoot = PendingRoot(block.timestamp, newRoot, newIpfsHash);
-            emit EventsLib.RootProposed(newRoot, newIpfsHash);
-        }
+    function submitRoot(bytes32 newRoot, bytes32 newIpfsHash) external onlyUpdaterRole {
+        require(newRoot != pendingRoot.root || newIpfsHash != pendingRoot.ipfsHash, ErrorsLib.ALREADY_PENDING);
+
+        pendingRoot = PendingRoot({root: newRoot, ipfsHash: newIpfsHash, validAt: block.timestamp + timelock});
+
+        emit EventsLib.PendingRootSet(msg.sender, newRoot, newIpfsHash);
     }
 
     /// @notice Accepts and sets the current pending merkle root.
     /// @dev This function can only be called after the timelock has expired.
     /// @dev Anyone can call this function.
     function acceptRoot() external {
-        require(pendingRoot.submittedAt > 0, ErrorsLib.NO_PENDING_ROOT);
-        require(block.timestamp >= pendingRoot.submittedAt + timelock, ErrorsLib.TIMELOCK_NOT_EXPIRED);
+        require(pendingRoot.validAt != 0, ErrorsLib.NO_PENDING_ROOT);
+        require(block.timestamp >= pendingRoot.validAt, ErrorsLib.TIMELOCK_NOT_EXPIRED);
 
-        root = pendingRoot.root;
-        ipfsHash = pendingRoot.ipfsHash;
+        _setRoot(pendingRoot.root, pendingRoot.ipfsHash);
+    }
 
-        emit EventsLib.RootSet(pendingRoot.root, pendingRoot.ipfsHash);
+    /// @notice Revokes the pending root.
+    /// @dev Can be frontrunned with `acceptRoot` in case the timelock has passed.
+    function revokePendingRoot() external onlyUpdaterRole {
+        require(pendingRoot.validAt != 0, ErrorsLib.NO_PENDING_ROOT);
 
         delete pendingRoot;
+
+        emit EventsLib.PendingRootRevoked(msg.sender);
     }
 
     /// @notice Claims rewards.
@@ -122,9 +124,9 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor {
             ErrorsLib.INVALID_PROOF_OR_EXPIRED
         );
 
-        amount = claimable - claimed[account][reward];
+        require(claimable > claimed[account][reward], ErrorsLib.CLAIMABLE_TOO_LOW);
 
-        require(amount > 0, ErrorsLib.ALREADY_CLAIMED);
+        amount = claimable - claimed[account][reward];
 
         claimed[account][reward] = claimable;
 
@@ -136,23 +138,21 @@ contract UniversalRewardsDistributor is IUniversalRewardsDistributor {
     /// @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 is 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);
     }
 
     /// @notice Sets the timelock of a given distribution.
     /// @param newTimelock The new timelock.
     /// @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.
+    /// @dev The timelock modification are not applicable to the pending values.
     function setTimelock(uint256 newTimelock) external onlyOwner {
-        if (newTimelock < timelock) {
-            require(
-                pendingRoot.submittedAt == 0 || pendingRoot.submittedAt + timelock <= block.timestamp,
-                ErrorsLib.TIMELOCK_NOT_EXPIRED
-            );
-        }
+        require(newTimelock != timelock, ErrorsLib.ALREADY_SET);
 
         _setTimelock(newTimelock);
     }
@@ -161,25 +161,17 @@ 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);
     }
 
-    /// @notice Revokes the pending root of a given distribution.
-    /// @dev This function can only be called by the owner of the distribution at any time.
-    /// @dev Can be frontrunned by triggering the `acceptRoot` function in case the timelock has passed. This if the
-    /// `owner` responsibility to trigger this function before the end of the timelock.
-    function revokeRoot() external onlyOwner {
-        require(pendingRoot.submittedAt != 0, ErrorsLib.NO_PENDING_ROOT);
-
-        delete pendingRoot;
-
-        emit EventsLib.RootRevoked();
-    }
-
     /// @notice Sets the `owner` of the distribution to `newOwner`.
     function setOwner(address newOwner) external onlyOwner {
+        require(newOwner != owner, ErrorsLib.ALREADY_SET);
+
         _setOwner(newOwner);
     }
 

src/UrdFactory.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
-pragma solidity 0.8.21;
+pragma solidity 0.8.19;
 
 import {EventsLib} from "./libraries/EventsLib.sol";