ADD-06 MitigationConfirmed

[c4-bot-5]

Lines of code

Vulnerability details

Vulnerability details

The mitigation purpose of ADD-06 is the following:

Mitigation for gas-intensive setUpAirdrop function and airdrop mechanism. 
Missing finalized test for new airdrop mechanism but working Airdrop script 
based on Merkle root and proof.

With this mitigation, developers want to reduce the gas cost of the Neuron.setupAirdrop() method.

Neuron.sol#L123-L134

123      /// @notice Sets up the allowance from the treasury address to transfer to each recipient address.
124      /// @dev Only admins are authorized to call this function.
125      /// @param recipients The array of recipient addresses
126      /// @param amounts The array of corresponding amounts for each recipient
127      function setupAirdrop(address[] calldata recipients, uint256[] calldata amounts) external {
128          require(isAdmin[msg.sender]);
129          require(recipients.length == amounts.length);
130          uint256 recipientsLength = recipients.length;
131          for (uint32 i = 0; i < recipientsLength; i++) {
132              _approve(treasuryAddress, recipients[i], amounts[i]);
133          }
134      }

This function can be used by admin to allow the recipient[i] to take an amount[i] from treasuryAddress. The purpose of this function is to give the admin role the possibility to make multiple approve actions.

Once a recipient has been approved, it could use the Neuron.claim() method to get the amount from treasuryAddress

Neuron.sol#L136-L145

136      /// @notice Claims the specified amount of tokens from the treasury address to the caller's address.
137      /// @param amount The amount to be claimed
138      function claim(uint256 amount) external {
139          require(
140              allowance(treasuryAddress, msg.sender) >= amount, 
141              "ERC20: claim amount exceeds allowance"
142          );
143          transferFrom(treasuryAddress, msg.sender, amount);
144          emit TokensClaimed(msg.sender, amount);
145      }

However, the mechanism above seems very gas-intensive, and developers wanted to mitigate it.

Mitigation implemented by developers

In pull #16 was implemented the mitigation and tests. It uses the Openzeppelin MerkleProof library.

Two state variables were introduced in Neuron.sol, airdropToMerkleRoot, and rootClaimedAirdrop:

Neuron.sol#L59-L63

59      /// @notice Airdrop kind to merkle tree root.
60      mapping(string => bytes32) public airdropToMerkleRoot;
61  
62      /// @notice Mapping of claime's and claimed status.
63      mapping(bytes32 => mapping(address => bool)) public rootClaimedAirdrop;

The variable airdropToMerkleRoot is initialized inside Neuron.constructor():

83          airdropToMerkleRoot["TGE"] = root;

and can be modified by _ownerAddress using Neuron.setMerkleRoot():

95      /// @notice Sets root of the merkle tree.
96      /// @dev Only the owner address is authorized to call this function.
97      /// @param airDropKind airdrop type.
98      /// @param root Root of the merkle tree.
99      function setMerkleRoot(string memory airDropKind, bytes32 root) external {
100          require(msg.sender == _ownerAddress);
101          airdropToMerkleRoot[airDropKind] = root;
102      }

Then, it is introduced a function to verify if a leaf, i.e. the hash of a tuple (address, amount), belongs to the Merkle tree: Neuron.verify():

232      function verify(
233          bytes32[] memory proof,
234          address addr,
235          uint256 amount,
236          string memory airdropKind
237      ) public {
238          bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(addr, amount))));
239          require(MerkleProof.verify(proof, airdropToMerkleRoot[airdropKind], leaf), "Invalid proof");
240          // ...
241      }

This function uses MerkleProof.verify(), which has this documentation:

MerkleProof.sol#L28-L36

28      /**
29       * @dev Returns true if a `leaf` can be proved to be a part of a Merkle tree
30       * defined by `root`. For this, a `proof` must be provided, containing
31       * sibling hashes on the branch from the leaf to the root of the tree. Each
32       * pair of leaves and each pair of pre-images are assumed to be sorted.
33       */
34      function verify(bytes32[] memory proof, bytes32 root, bytes32 leaf) internal pure returns (bool) {
35          return processProof(proof, leaf) == root;
36      }

According to my conversation with developers, they will build the Merkle tree off-chain using a group of tuples (address, amount) as leaves. Then, they will put the Merkle root in airdropToMerkleRoot, in the wanted airdropKind. They will supply the proof to addresses that belong to the Merkle tree, making the recipients able to call the new Neuron.claim():

154      /// @notice Claims the specified amount of tokens from the treasury address to the caller's address.
155      /// @param airdropKind The kind of airdrop to be claimed
156      /// @param proof The proof of the claimee
157      /// @param amount The amount to be claimed
158      function claim( 
159          string memory airdropKind,
160          bytes32[] memory proof,
161          uint256 amount
162      ) 
163          external 
164      {
165          require(!rootClaimedAirdrop[airdropToMerkleRoot[airdropKind]][msg.sender], "Already claimed airdrop");
166          verify(proof, msg.sender, amount, airdropKind);
167          rootClaimedAirdrop[airdropToMerkleRoot[airdropKind]][msg.sender] = true;
168          _approve(treasuryAddress, msg.sender, amount);
169          transferFrom(treasuryAddress, msg.sender, amount);
170          emit TokensClaimed(msg.sender, amount);
171      }

In this way, an address would be able to call successfully Neuron.claim() only if he/she has the proof for the right airdropKind and the tuple (address, amount) is a leaf of that Merkle tree.

Comment about the Mitigation Proposal

Using the new implementation, developers save gas. Now admin hasn't to approve anything. The recipient with the right proof can claim their amounts autonomously. However, we asked developers if some scenarios are possible, and we want to report our considerations about them.

Unclaimed airdrop

Even though it is easy to check if an address has already claimed the airdrop for a specific airdropKind thanks to the rootClaimedAirdrop, it could be harder to check if an airdropKind has still unclaimed airdrops. This fact could become critical if we assume the possibility of changing the Merkle root of an airdropKind when there are unclaimed airdrops in it.

To check if an airdropKind has still unclaimed airdrops, admin, which should know all leaves of the corresponding Merkle tree, could check the value of rootClaimedAirdrop for all of them and verify they are all true. However, we suggest to add a counter to make this operation simpler:

@@ -43,6 +43,7 @@ contract Neuron is ERC20, AccessControl {
     /// @notice Maximum supply of NRN tokens.
     uint256 public constant MAX_SUPPLY = 10**18 * 10**9;

+
     /// @notice The address of treasury.
     address public treasuryAddress;

@@ -62,6 +63,8 @@ contract Neuron is ERC20, AccessControl {
     /// @notice Mapping of claime's and claimed status.
     mapping(bytes32 => mapping(address => bool)) public rootClaimedAirdrop;

+    mapping(string => uint256) public airdropClaimedCounter;
+
     /*//////////////////////////////////////////////////////////////
                                CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -99,6 +102,7 @@ contract Neuron is ERC20, AccessControl {
     function setMerkleRoot(string memory airDropKind, bytes32 root) external {
         require(msg.sender == _ownerAddress);
         airdropToMerkleRoot[airDropKind] = root;
+        airdropClaimedCounter[airDropKind] = 0;
     }

     /// @notice Sets the treasury address.
@@ -165,6 +169,7 @@ contract Neuron is ERC20, AccessControl {
         require(!rootClaimedAirdrop[airdropToMerkleRoot[airdropKind]][msg.sender], "Already claimed airdrop");
         verify(proof, msg.sender, amount, airdropKind);
         rootClaimedAirdrop[airdropToMerkleRoot[airdropKind]][msg.sender] = true;
+        airdropClaimedCounter[airDropKind]++;
         _approve(treasuryAddress, msg.sender, amount);
         transferFrom(treasuryAddress, msg.sender, amount);
         emit TokensClaimed(msg.sender, amount);

In this case, we strongly suggest avoiding adding a function to check if airdropClaimedCounter is equal to an expected value, because this call would make public the number of leaves of the Merkle tree.

Multiple claims by the same recipient

Every airdropKind is bound with a Merkle tree through the merkle tree root. There could be the case that the admin would build a new merkle tree. Let's describe this scenario. The tuple (Bob, amount) is a leaf of the current Merkle tree for airdropKind = "TGE".

1) Bob decides to claim it and he obtains the amount. Now, airdropToMerkleRoot["TGE"]][Bob]=true. 2) admin wants to change another tuple, (Alice, amount0) with (Alice, amount1). So admin decides to build a new Merkle tree with the same tuples, changing only Alice's one. Bob's one is inserted in the new Merkle tree, without any modification. Then, admin set the new Merkle root in airdropToMerkleRoot["TGE"] 3) Because the Merkle root is changed, airdropToMerkleRoot["TGE"]][Bob]=false: Bob can claim the amount again.

If the scenario above is unwanted and developers think of changing the Merkle root of a tree that was partially unclaimed, we suggest using the airdropKind as the airdropToMerkleRoot key, instead of the Merkle root.

@@ -60,7 +60,9 @@ contract Neuron is ERC20, AccessControl {
     mapping(string => bytes32) public airdropToMerkleRoot;

     /// @notice Mapping of claime's and claimed status.
-    mapping(bytes32 => mapping(address => bool)) public rootClaimedAirdrop;
+    mapping(string => mapping(address => bool)) public rootClaimedAirdrop;
+
+    mapping(string => bool) public airdropID_active;

     /*//////////////////////////////////////////////////////////////
                                CONSTRUCTOR
@@ -94,11 +96,16 @@ contract Neuron is ERC20, AccessControl {

     /// @notice Sets root of the merkle tree.
     /// @dev Only the owner address is authorized to call this function.
-    /// @param airDropKind airdrop type.
+    /// @param airDropKindID airdrop type.
     /// @param root Root of the merkle tree.
-    function setMerkleRoot(string memory airDropKind, bytes32 root) external {
+    function setMerkleRoot(string memory airDropKindID, bytes32 root) external {
+        require(msg.sender == _ownerAddress);
+        airdropToMerkleRoot[airDropKindID] = root;
+    }
+
+    function adjustAirdropID(string memory airDropKindID, bool active) external {
         require(msg.sender == _ownerAddress);
-        airdropToMerkleRoot[airDropKind] = root;
+        airdropID_active[airDropKindID] = active;
     }--

     /// @notice Sets the treasury address.
@@ -152,19 +159,20 @@ contract Neuron is ERC20, AccessControl {
     }

     /// @notice Claims the specified amount of tokens from the treasury address to the caller's address.
-    /// @param airdropKind The kind of airdrop to be claimed
+    /// @param airdropKindID The kind of airdrop to be claimed
     /// @param proof The proof of the claimee
     /// @param amount The amount to be claimed
     function claim(
-        string memory airdropKind,
+        string memory airdropKindID,
         bytes32[] memory proof,
         uint256 amount
     ) -
         external
     {--
-        require(!rootClaimedAirdrop[airdropToMerkleRoot[airdropKind]][msg.sender], "Already claimed airdrop");
-        verify(proof, msg.sender, amount, airdropKind);
-        rootClaimedAirdrop[airdropToMerkleRoot[airdropKind]][msg.sender] = true;
+        require(!rootClaimedAirdrop[airdropKindID][msg.sender], "Already claimed airdrop");
+        require(airdropID_active[airDropKindID]);
+        verify(proof, msg.sender, amount, airdropKindID);
+        rootClaimedAirdrop[airdropKindID][msg.sender] = true;
         _approve(treasuryAddress, msg.sender, amount);
         transferFrom(treasuryAddress, msg.sender, amount);
         emit TokensClaimed(msg.sender, amount);

In the mitigation proposed above, a unique identifier is used for airDropKindID. We suggest using "TGE_V1", "TGE_V2", "TGE_V3" and so on when you decide to build a new Merkle tree and use the airdropID_active to deactivate old Merkle roots.

Lack of transparency

Before mitigation, the list of approved addresses was public, because it was on-chain. Now, the Merkle trees are built off-chain and users can't see them. If developers think transparency could be an important aspect, we suggest rolling back the code. Furthermore, because Ai Arena will be released on Arbitrum, it could not waste too much gas.

[c4-judge]

jhsagd76 marked the issue as satisfactory