address review comments

WIN-000

contracts/Batcher.sol

@@ -71,12 +71,12 @@ contract Batcher {
     // Revert everything if any transfer fails
     for (uint8 i = 0; i < recipients.length; i++) {
       require(recipients[i] != address(0), 'Invalid recipient address');
+      emit BatchTransfer(msg.sender, recipients[i], values[i]);
       (bool success, ) = recipients[i].call{
         value: values[i],
         gas: transferGasLimit
       }('');
       require(success, 'Send failed');
-      emit BatchTransfer(msg.sender, recipients[i], values[i]);
     }
   }
 
@@ -91,6 +91,7 @@ contract Batcher {
     uint256 value,
     bytes calldata data
   ) external onlyOwner returns (bytes memory) {
+    require(to != address(0), 'Invalid recipient address');
     (bool success, bytes memory returnData) = to.call{ value: value }(data);
     require(success, 'Recover failed');
     return returnData;

contracts/Forwarder.sol

@@ -39,14 +39,13 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
       return;
     }
 
-    (bool success, ) = parentAddress.call{ value: value }('');
-    require(success, 'Flush failed');
-
     // NOTE: since we are forwarding on initialization,
     // we don't have the context of the original sender.
     // We still emit an event about the forwarding but set
     // the sender to the forwarder itself
     emit ForwarderDeposited(address(this), value, msg.data);
+    (bool success, ) = parentAddress.call{ value: value }('');
+    require(success, 'Flush failed');
   }
 
   /**
@@ -136,6 +135,7 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
     uint256 value,
     bytes calldata data
   ) external onlyParent returns (bytes memory) {
+    require(target != address(0), 'Invalid target address');
     (bool success, bytes memory returnedData) = target.call{ value: value }(
       data
     );
@@ -303,9 +303,9 @@ contract Forwarder is IERC721Receiver, ERC1155Receiver, IForwarder {
       return;
     }
 
+    emit ForwarderDeposited(msg.sender, value, msg.data);
     (bool success, ) = parentAddress.call{ value: value }('');
     require(success, 'Flush failed');
-    emit ForwarderDeposited(msg.sender, value, msg.data);
   }
 
   /**

contracts/ForwarderFactory.sol

@@ -4,7 +4,7 @@ import './Forwarder.sol';
 import './CloneFactory.sol';
 
 contract ForwarderFactory is CloneFactory {
-  address public implementationAddress;
+  address public immutable implementationAddress;
 
   event ForwarderCreated(
     address newForwarderAddress,
@@ -14,6 +14,7 @@ contract ForwarderFactory is CloneFactory {
   );
 
   constructor(address _implementationAddress) {
+    require(_implementationAddress != address(0), 'Invalid implementation address');
     implementationAddress = _implementationAddress;
   }
 
@@ -31,13 +32,13 @@ contract ForwarderFactory is CloneFactory {
     bytes32 finalSalt = keccak256(abi.encodePacked(parent, salt));
 
     address payable clone = createClone(implementationAddress, finalSalt);
-    Forwarder(clone).init(
+    emit ForwarderCreated(
+      clone,
       parent,
       shouldAutoFlushERC721,
       shouldAutoFlushERC1155
     );
-    emit ForwarderCreated(
-      clone,
+    Forwarder(clone).init(
       parent,
       shouldAutoFlushERC721,
       shouldAutoFlushERC1155

contracts/ForwarderFactoryV4.sol

@@ -8,7 +8,7 @@ import './CloneFactory.sol';
  * @notice This contract will deploy new forwarder contracts using the create2 opcode
  */
 contract ForwarderFactoryV4 is CloneFactory {
-  address public implementationAddress;
+  address public immutable implementationAddress;
 
   /**
    * @notice Event triggered when a new forwarder is deployed
@@ -31,6 +31,7 @@ contract ForwarderFactoryV4 is CloneFactory {
    * @param _implementationAddress Address of the current forwarder implementation
    */
   constructor(address _implementationAddress) {
+    require(_implementationAddress != address(0), 'Invalid implementation address');
     implementationAddress = _implementationAddress;
   }
 
@@ -67,14 +68,14 @@ contract ForwarderFactoryV4 is CloneFactory {
     bytes32 finalSalt = keccak256(abi.encodePacked(parent, feeAddress, salt));
 
     address payable clone = createClone(implementationAddress, finalSalt);
-    ForwarderV4(clone).init(
+    emit ForwarderCreated(
+      clone,
       parent,
       feeAddress,
       shouldAutoFlushERC721,
       shouldAutoFlushERC1155
     );
-    emit ForwarderCreated(
-      clone,
+    ForwarderV4(clone).init(
       parent,
       feeAddress,
       shouldAutoFlushERC721,

contracts/ForwarderV4.sol

@@ -89,16 +89,15 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
       return;
     }
 
-    (bool success, ) = parentAddress.call{ value: value }('');
-    require(success, 'Flush failed');
-
     /**
      * Since we are forwarding on initialization,
      * we don't have the context of the original sender.
      * We still emit an event about the forwarding but set
      * the sender to the forwarder itself
      */
     emit ForwarderDeposited(address(this), value, msg.data);
+    (bool success, ) = parentAddress.call{ value: value }('');
+    require(success, 'Flush failed');
   }
 
   /**
@@ -165,6 +164,7 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
     bytes calldata data
   ) external returns (bytes memory) {
     require(msg.sender == parentAddress, 'Only Parent');
+    require(target != address(0), 'Invalid target address');
     (bool success, bytes memory returnedData) = target.call{ value: value }(
       data
     );
@@ -359,9 +359,9 @@ contract ForwarderV4 is IERC721Receiver, ERC1155Receiver, IForwarderV4 {
       return;
     }
 
+    emit ForwarderDeposited(msg.sender, value, msg.data);
     (bool success, ) = parentAddress.call{ value: value }('');
     require(success, 'Flush failed');
-    emit ForwarderDeposited(msg.sender, value, msg.data);
   }
 
   /**

contracts/WalletFactory.sol

@@ -4,11 +4,12 @@ import './WalletSimple.sol';
 import './CloneFactory.sol';
 
 contract WalletFactory is CloneFactory {
-  address public implementationAddress;
+  address public immutable implementationAddress;
 
   event WalletCreated(address newWalletAddress, address[] allowedSigners);
 
   constructor(address _implementationAddress) {
+    require(_implementationAddress != address(0), 'Invalid implementation address');
     implementationAddress = _implementationAddress;
   }
 
@@ -19,7 +20,7 @@ contract WalletFactory is CloneFactory {
     bytes32 finalSalt = keccak256(abi.encodePacked(allowedSigners, salt));
 
     address payable clone = createClone(implementationAddress, finalSalt);
-    WalletSimple(clone).init(allowedSigners);
     emit WalletCreated(clone, allowedSigners);
+    WalletSimple(clone).init(allowedSigners);
   }
 }

contracts/WalletSimple.sol

@@ -80,6 +80,8 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
 
     for (uint8 i = 0; i < allowedSigners.length; i++) {
       require(allowedSigners[i] != address(0), 'Invalid signer');
+      require(!signers[allowedSigners[i]], 'Duplicate signer');
+
       signers[allowedSigners[i]] = true;
     }
 
@@ -189,7 +191,7 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
   ) external onlySigner {
     // Verify the other signer
     bytes32 operationHash = keccak256(
-      abi.encodePacked(
+      abi.encode(
         getNetworkId(),
         toAddress,
         value,
@@ -207,10 +209,6 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
       sequenceId
     );
 
-    // Success, send the transaction
-    (bool success, ) = toAddress.call{ value: value }(data);
-    require(success, 'Call execution failed');
-
     emit Transacted(
       msg.sender,
       otherSigner,
@@ -219,6 +217,10 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
       value,
       data
     );
+    require(toAddress != address(0), 'Invalid destination address');
+    // Success, send the transaction
+    (bool success, ) = toAddress.call{ value: value }(data);
+    require(success, 'Call execution failed');
   }
 
   /**
@@ -248,7 +250,7 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
 
     // Verify the other signer
     bytes32 operationHash = keccak256(
-      abi.encodePacked(
+      abi.encode(
         getBatchNetworkId(),
         recipients,
         values,
@@ -268,8 +270,8 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
       sequenceId
     );
 
-    batchTransfer(recipients, values);
     emit BatchTransacted(msg.sender, otherSigner, operationHash);
+    batchTransfer(recipients, values);
   }
 
   /**
@@ -282,14 +284,13 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
   function batchTransfer(
     address[] calldata recipients,
     uint256[] calldata values
-  ) internal {
+  ) private {
     for (uint256 i = 0; i < recipients.length; i++) {
       require(address(this).balance >= values[i], 'Insufficient funds');
 
+      emit BatchTransfer(msg.sender, recipients[i], values[i]);
       (bool success, ) = recipients[i].call{ value: values[i] }('');
       require(success, 'Call failed');
-
-      emit BatchTransfer(msg.sender, recipients[i], values[i]);
     }
   }
 
@@ -314,7 +315,7 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
   ) external onlySigner {
     // Verify the other signer
     bytes32 operationHash = keccak256(
-      abi.encodePacked(
+      abi.encode(
         getTokenNetworkId(),
         toAddress,
         value,
@@ -553,7 +554,7 @@ contract WalletSimple is IERC721Receiver, ERC1155Receiver {
    * greater than the minimum element in the window.
    * @param sequenceId to insert into array of stored ids
    */
-  function tryInsertSequenceId(uint256 sequenceId) private onlySigner {
+  function tryInsertSequenceId(uint256 sequenceId) private {
     // Keep a pointer to the lowest value element in the window
     uint256 lowestValueIndex = 0;
     // fetch recentSequenceIds into memory for function context to avoid unnecessary sloads

contracts/recoveryContracts/RecoveryWalletFactory.sol

@@ -7,6 +7,7 @@ contract RecoveryWalletFactory is CloneFactory {
   address public implementationAddress;
 
   constructor(address _implementationAddress) {
+    require(_implementationAddress != address(0), 'Invalid implementation address');
     implementationAddress = _implementationAddress;
   }
 

contracts/recoveryContracts/RecoveryWalletSimple.sol

@@ -20,6 +20,7 @@ contract RecoveryWalletSimple is IERC721Receiver, ERC1155Receiver {
   bool public initialized = false; // True if the contract has been initialized
 
   function init(address _signer) external onlyUninitialized {
+    require(signer != address(0), 'Invalid signer address');
     signer = _signer;
     initialized = true;
   }
@@ -62,6 +63,7 @@ contract RecoveryWalletSimple is IERC721Receiver, ERC1155Receiver {
     uint256 value,
     bytes calldata data
   ) external onlySigner {
+    require(toAddress != address(0), 'Invalid destination address');
     // Success, send the transaction
     (bool success, ) = toAddress.call{ value: value }(data);
     require(success, 'Call execution failed');

hardhat.config.ts

@@ -27,7 +27,16 @@ const {
 } = process.env;
 
 const config: HardhatUserConfig = {
-  solidity: '0.8.20',
+  solidity: {
+    compilers: [
+      {
+          version: '0.8.20',
+          settings: {
+              evmVersion: 'paris'
+          }
+      }
+    ]
+  },
   networks: {
     hardhat: {
       // If chainId is omitted, then there is no chain id validation

package.json

@@ -25,7 +25,7 @@
     "ethereum"
   ],
   "dependencies": {
-    "@openzeppelin/contracts": "4.9.0",
+    "@openzeppelin/contracts": "5.0.0",
     "@truffle/hdwallet-provider": "^2.0.0",
     "bignumber.js": "^9.0.0",
     "bluebird": "^3.3.5",