Connext contest - hrkrshnn's results

Handle

hrkrshnn

Vulnerability details

Increment in the loop can be made unchecked

 import "./interfaces/IFulfillHelper.sol";
 import "./interfaces/ITransactionManager.sol";
@@ -556,6 +556,12 @@ contract TransactionManager is ReentrancyGuard, ITransactionManager {
     return activeTransactionBlocks[user];
   }

+  function unchecked_inc(uint256 i) internal pure returns (uint256) {
+      unchecked {
+          return i + 1;
+      }
+  }
+
   //////////////////////////
   /// Private functions ///
   //////////////////////////
@@ -570,7 +576,7 @@ contract TransactionManager is ReentrancyGuard, ITransactionManager {
     uint256[] memory updated = new uint256[](newLength);
     bool removed = false;
     uint256 updatedIdx = 0;
-    for (uint256 i; i < newLength + 1; i++) {
+    for (uint256 i; i < newLength + 1; i = unchecked_inc(i)) {
       // Handle case where there could be more than one tx added in a block
       // And only one should be removed
       if (!removed && activeTransactionBlocks[user][i] == preparedBlock) {

Make sure that you enable the optimizer and need at least solidity 0.8.4 (i.e., the low level inliner.)

The old code would have an unnecessary checked addition, and potentially also, not inlined. Would save at least 20-30 gas for each loop iteration.

Handle

hrkrshnn

Vulnerability details

Revert strings

Consider using custom errors instead of revert strings

Can save gas when the revert condition has been met. And also during runtime.

Consider shortening revert strings to less than 32 bytes

Revert strings more than 32 bytes require at least one additional mstore, along with additional operations for computing memory offset, etc.

Even if you need a string to represent an error, it can usually be done in less than 32 bytes / characters.

Here are some examples of strings that can be shortened from codebase:

./contracts/TransactionManager.sol:96:      "addLiquidity: ETH_WITH_ERC_TRANSFER"
./contracts/TransactionManager.sol:97:      "addLiquidity: ERC20_TRANSFER_FAILED"
./contracts/TransactionManager.sol:122:     "removeLiquidity: INSUFFICIENT_FUNDS"

Note that this will only decrease runtime gas when the revert condition has been met. Regardless, it will decrease deploy time gas.

Handle

hrkrshnn

Vulnerability details

Optimizing the for loop

The for loop can improved, here is a diff:

@@ -565,22 +565,26 @@ contract TransactionManager is ReentrancyGuard, ITransactionManager {
   /// @param user User who has completed a transaction
   /// @param preparedBlock The TransactionData.preparedBlockNumber to remove
   function removeUserActiveBlocks(address user, uint256 preparedBlock) internal {
-    // Remove active blocks
-    uint256 newLength = activeTransactionBlocks[user].length - 1;
-    uint256[] memory updated = new uint256[](newLength);
-    bool removed = false;
-    uint256 updatedIdx = 0;
-    for (uint256 i; i < newLength + 1; i++) {
-      // Handle case where there could be more than one tx added in a block
-      // And only one should be removed
-      if (!removed && activeTransactionBlocks[user][i] == preparedBlock) {
-        removed = true;
-        continue;
-      }
-      updated[updatedIdx] = activeTransactionBlocks[user][i];
-      updatedIdx++;
+
+    uint256[] storage array = activeTransactionBlocks[user];
+    uint256 length = array.length;
+    uint256 matchIdx = type(uint).max;
+
+    for (uint256 i = 0; i < length; i++) {
+        if (array[i] == preparedBlock)
+        {
+            matchIdx = i;
+            break;
+        }
     }
-    activeTransactionBlocks[user] = updated;
+
+    if (matchIdx != type(uint256).max) {
+        for (uint256 i = matchIdx; i < length; i++) {
+            array[i] = array[i+1];
+        }
+        array.pop();
+    }
+
   }

The other implementation creates unnecessary copies in memory. And overwrites slots unnecessary (something like sstore(slot, sload(slot))). The above implementation should save significant amount of gas, by avoiding both unnecessary memory and unnecessary storage writes. Please check; was written in a hurry, but the general idea should work :)

Optimizing the loop: Try to pass the array index instead

Instead of trying to compute the index i where

activeTransactionBlocks[user][i] == blockIndex

One can try to already compute this off chain and pass it as the parameter.

This avoids the expensive step of reading values from storage on chain, and saves a significant amount of gas.

Assume that this index i is passed as a parameter. On chain, all you need to do is have

require(activeTransactionBlocks[user][i] == blockIndex)

After that, use the second for loop at the end in the previous example.