Added Test Cases per Zellic audit questions

contracts/smart-account/modules/BatchedSessionRouterModule.sol

@@ -83,6 +83,8 @@ contract BatchedSessionRouter is BaseAuthorizationModule {
         ) = abi.decode(userOp.callData[4:], (address[], uint256[], bytes[]));
 
         uint256 length = sessionData.length;
+        require(length == destinations.length, "Lengths mismatch");
+
         // iterate over batched operations
         for (uint256 i; i < length; ) {
             // validate the sessionKey

contracts/smart-account/modules/SessionValidationModules/ERC20SessionValidationModule.sol

@@ -21,6 +21,8 @@ contract ERC20SessionValidationModule is ISessionValidationModule {
      * @param callValue value to be sent with the call
      * @param _funcCallData the data for the call. is parsed inside the SVM
      * @param _sessionKeyData SessionKey data, that describes sessionKey permissions
+     * param _callSpecificData additional data, for example some proofs if the SVM utilizes merkle trees itself
+     * for example to store a list of allowed tokens or receivers
      */
     function validateSessionParams(
         address destinationContract,
@@ -70,7 +72,6 @@ contract ERC20SessionValidationModule is ISessionValidationModule {
                 bytes4(_op.callData[0:4]) == EXECUTE_SELECTOR,
             "ERC20SV Invalid Selector"
         );
-
         (
             address sessionKey,
             address token,
@@ -94,20 +95,29 @@ contract ERC20SessionValidationModule is ISessionValidationModule {
         // working with userOp.callData
         // check if the call is to the allowed recepient and amount is not more than allowed
         bytes calldata data;
+
         {
+            //offset represents where does the inner bytes array start
             uint256 offset = uint256(bytes32(_op.callData[4 + 64:4 + 96]));
             uint256 length = uint256(
                 bytes32(_op.callData[4 + offset:4 + offset + 32])
             );
             //we expect data to be the `IERC20.transfer(address, uint256)` calldata
             data = _op.callData[4 + offset + 32:4 + offset + 32 + length];
         }
-        if (address(bytes20(data[16:36])) != recipient) {
+
+        (address recipientCalled, uint256 amount) = abi.decode(
+            data[4:],
+            (address, uint256)
+        );
+
+        if (recipientCalled != recipient) {
             revert("ERC20SV Wrong Recipient");
         }
-        if (uint256(bytes32(data[36:68])) > maxAmount) {
+        if (amount > maxAmount) {
             revert("ERC20SV Max Amount Exceeded");
         }
+
         return
             ECDSA.recover(
                 ECDSA.toEthSignedMessageHash(_userOpHash),

test/module/BatchedSessionRouter.Module.specs.ts

@@ -1,4 +1,4 @@
-import { expect, use } from "chai";
+import { expect } from "chai";
 import {
   enableNewTreeForSmartAccountViaEcdsa,
   getERC20SessionKeyParams,
@@ -17,7 +17,7 @@ import {
 } from "../utils/setupHelper";
 import { computeAddress } from "ethers/lib/utils";
 
-describe("SessionKey: Session Router", async () => {
+describe("SessionKey: Batched Session Router", async () => {
   const [
     deployer,
     smartAccountOwner,
@@ -371,7 +371,7 @@ describe("SessionKey: Session Router", async () => {
         [approveCallData, interactCallData],
       ],
       userSA.address,
-      nonAuthSessionKey,
+      nonAuthSessionKey, // not authorized session key
       entryPoint,
       sessionKeyManager.address,
       [
@@ -402,6 +402,109 @@ describe("SessionKey: Session Router", async () => {
       .withArgs(0, "AA24 signature error");
   });
 
+  it("Should revert when sessionData array is empty", async () => {
+    const {
+      entryPoint,
+      userSA,
+      sessionKeyManager,
+      sessionRouter,
+      mockProtocol,
+      mockToken,
+    } = await setupTests();
+    const tokenAmountToTransfer = ethers.utils.parseEther("1.7534");
+
+    const MockProtocol = await ethers.getContractFactory("MockProtocol");
+    const IERC20 = await ethers.getContractFactory("ERC20");
+
+    const approveCallData = IERC20.interface.encodeFunctionData("approve", [
+      mockProtocol.address,
+      tokenAmountToTransfer,
+    ]);
+    const interactCallData = MockProtocol.interface.encodeFunctionData(
+      "interact",
+      [mockToken.address, tokenAmountToTransfer]
+    );
+
+    const userOp = await makeEcdsaSessionKeySignedBatchUserOp(
+      "executeBatch_y6U",
+      [
+        [mockToken.address, mockProtocol.address],
+        [0, 0],
+        [approveCallData, interactCallData],
+      ],
+      userSA.address,
+      sessionKey, 
+      entryPoint,
+      sessionKeyManager.address,
+      [], //empty session data array
+      sessionRouter.address
+    );
+
+    await expect(
+      entryPoint.handleOps([userOp], alice.address, { gasLimit: 10000000 })
+    )
+      .to.be.revertedWith("FailedOp")
+      .withArgs(0, "AA23 reverted: Lengths mismatch");
+  });
+
+  it("Should revert if not enough session datas provided", async () => {
+    const {
+      entryPoint,
+      userSA,
+      sessionKeyManager,
+      erc20SessionModule,
+      sessionKeyData,
+      leafData,
+      merkleTree,
+      sessionRouter,
+      mockProtocol,
+      mockToken,
+    } = await setupTests();
+    const tokenAmountToTransfer = ethers.utils.parseEther("1.7534");
+
+    const MockProtocol = await ethers.getContractFactory("MockProtocol");
+    const IERC20 = await ethers.getContractFactory("ERC20");
+
+    const approveCallData = IERC20.interface.encodeFunctionData("approve", [
+      mockProtocol.address,
+      tokenAmountToTransfer,
+    ]);
+    const interactCallData = MockProtocol.interface.encodeFunctionData(
+      "interact",
+      [mockToken.address, tokenAmountToTransfer]
+    );
+
+    const userOp = await makeEcdsaSessionKeySignedBatchUserOp(
+      "executeBatch_y6U",
+      [
+        [mockToken.address, mockProtocol.address],
+        [0, 0],
+        [approveCallData, interactCallData],
+      ],
+      userSA.address,
+      sessionKey,
+      entryPoint,
+      sessionKeyManager.address,
+      [
+        [
+          0,
+          0,
+          erc20SessionModule.address,
+          sessionKeyData,
+          merkleTree.getHexProof(ethers.utils.keccak256(leafData)),
+          "0x",
+        ],
+      ],
+      sessionRouter.address
+    );
+
+    await expect(
+      entryPoint.handleOps([userOp], alice.address, { gasLimit: 10000000 })
+    )
+      .to.be.revertedWith("FailedOp")
+      .withArgs(0, "AA23 reverted: Lengths mismatch");
+  });
+
   it("Should revert when when at least one SVM permission is violated", async () => {
     const {
       entryPoint,
@@ -695,7 +798,6 @@ describe("SessionKey: Session Router", async () => {
       entryPoint,
       userSA,
       sessionKeyManager,
-      erc20SessionModule,
       sessionKeyData,
       leafData,
       merkleTree,
@@ -705,7 +807,6 @@ describe("SessionKey: Session Router", async () => {
       mockToken,
       sessionKeyData2,
       leafData2,
-      validUntilForMockProtocol,
     } = await setupTests();
     const tokenAmountToTransfer = ethers.utils.parseEther("1.7534");
 
@@ -770,7 +871,6 @@ describe("SessionKey: Session Router", async () => {
       userSA,
       sessionKeyManager,
       erc20SessionModule,
-      sessionKeyData,
       leafData,
       merkleTree,
       sessionRouter,
@@ -779,7 +879,6 @@ describe("SessionKey: Session Router", async () => {
       mockToken,
       sessionKeyData2,
       leafData2,
-      validUntilForMockProtocol,
     } = await setupTests();
     const tokenAmountToTransfer = ethers.utils.parseEther("1.7534");
 

test/module/SessionValidationModules/ERC20SessionValidation.Module.specs.ts

@@ -371,6 +371,167 @@ describe("SessionKey: ERC20 Session Validation Module", async () => {
     );
   });
 
+  it("should revert callData is less than 4 bytes", async () => {
+    const {
+      entryPoint,
+      userSA,
+      sessionKeyManager,
+      erc20SessionModule,
+      mockToken,
+      sessionKeyData,
+      leafData,
+      merkleTree,
+    } = await setupTests();
+    const tokenAmountToTransfer = ethers.utils.parseEther("0.7534");
+
+    const charlieTokenBalanceBefore = await mockToken.balanceOf(
+      charlie.address
+    );
+
+    const transferUserOp = await makeEcdsaSessionKeySignedUserOp(
+      "execute_ncC",
+      [
+        mockToken.address,
+        ethers.utils.parseEther("0"),
+        encodeTransfer(charlie.address, tokenAmountToTransfer.toString()),
+      ],
+      userSA.address,
+      sessionKey,
+      entryPoint,
+      sessionKeyManager.address,
+      0,
+      0,
+      erc20SessionModule.address,
+      sessionKeyData,
+      merkleTree.getHexProof(ethers.utils.keccak256(leafData))
+    );
+
+    transferUserOp.callData = "0x1234"; // set callData to less than 4bytes
+
+    await expect(
+      entryPoint.handleOps([transferUserOp], alice.address, {
+        gasLimit: 10000000,
+      })
+    )
+      .to.be.revertedWith("FailedOp")
+      .withArgs(0, "AA23 reverted (or OOG)");
+    expect(await mockToken.balanceOf(charlie.address)).to.equal(
+      charlieTokenBalanceBefore
+    );
+  });
+
+  it("should revert callData is less than 4+32+32=68 bytes", async () => {
+    const {
+      entryPoint,
+      userSA,
+      sessionKeyManager,
+      erc20SessionModule,
+      mockToken,
+      sessionKeyData,
+      leafData,
+      merkleTree,
+    } = await setupTests();
+    const tokenAmountToTransfer = ethers.utils.parseEther("0.7534");
+
+    const charlieTokenBalanceBefore = await mockToken.balanceOf(
+      charlie.address
+    );
+
+    const transferUserOp = await makeEcdsaSessionKeySignedUserOp(
+      "execute_ncC",
+      [
+        mockToken.address,
+        ethers.utils.parseEther("0"),
+        encodeTransfer(charlie.address, tokenAmountToTransfer.toString()),
+      ],
+      userSA.address,
+      sessionKey,
+      entryPoint,
+      sessionKeyManager.address,
+      0,
+      0,
+      erc20SessionModule.address,
+      sessionKeyData,
+      merkleTree.getHexProof(ethers.utils.keccak256(leafData))
+    );
+
+    // trim userOp.callData to 66 bytes
+    transferUserOp.callData = transferUserOp.callData.toString().slice(0, 132); // 132 = 66*2
+
+    await expect(
+      entryPoint.handleOps([transferUserOp], alice.address, {
+        gasLimit: 10000000,
+      })
+    )
+      .to.be.revertedWith("FailedOp")
+      .withArgs(0, "AA23 reverted (or OOG)");
+    expect(await mockToken.balanceOf(charlie.address)).to.equal(
+      charlieTokenBalanceBefore
+    );
+  });
+
+  it("should revert if userOp.callData does not feature the correct length of execute() calldata argument", async () => {
+    const {
+      entryPoint,
+      userSA,
+      sessionKeyManager,
+      erc20SessionModule,
+      mockToken,
+      sessionKeyData,
+      leafData,
+      merkleTree,
+    } = await setupTests();
+    const tokenAmountToTransfer = ethers.utils.parseEther("0.7534");
+
+    const charlieTokenBalanceBefore = await mockToken.balanceOf(
+      charlie.address
+    );
+
+    const transferUserOp = await makeEcdsaSessionKeySignedUserOp(
+      "execute_ncC",
+      [
+        mockToken.address,
+        ethers.utils.parseEther("0"),
+        encodeTransfer(charlie.address, tokenAmountToTransfer.toString()),
+      ],
+      userSA.address,
+      sessionKey,
+      entryPoint,
+      sessionKeyManager.address,
+      0,
+      0,
+      erc20SessionModule.address,
+      sessionKeyData,
+      merkleTree.getHexProof(ethers.utils.keccak256(leafData))
+    );
+
+    const callDataString = transferUserOp.callData.toString().slice(2,);
+
+    const offset = 
+    parseInt(
+      (callDataString.slice(68*2, 100*2)),
+      16
+    );      
+    const length = parseInt(
+      (callDataString.slice((4+offset)*2, (4+offset+32)*2)),
+      16
+    );
+
+    const newLength = ethers.utils.hexZeroPad(ethers.utils.hexlify(length+1), 32);
+    transferUserOp.callData = transferUserOp.callData.toString().slice(0, (4+offset)*2) + newLength.slice(2,) + transferUserOp.callData.toString().slice((4 + offset + 32)*2);
+
+    await expect(
+      entryPoint.handleOps([transferUserOp], alice.address, {
+        gasLimit: 10000000,
+      })
+    )
+      .to.be.revertedWith("FailedOp")
+      .withArgs(0, "AA23 reverted (or OOG)"); // reverts when trying to abi.decode the callData[4:] to (address, uint256, bytes) since bytes length is incorrect
+    expect(await mockToken.balanceOf(charlie.address)).to.equal(
+      charlieTokenBalanceBefore
+    );
+  });
+
   it("Should not allow to call methods except execute or execute_ncC", async () => {
     const {
       entryPoint,