test: [QS ALC-9] add more tests for insufficient return data and clar…

…ifying comments (#277)
alchemyplatform · Nov 13, 2024 · deb1a2d · deb1a2d
1 parent c23dec6
commit deb1a2d
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 2 deletions.
diff --git a/src/libraries/ExecutionLib.sol b/src/libraries/ExecutionLib.sol
@@ -258,6 +258,7 @@ library ExecutionLib {
 
             success :=
                 and(
+                    // Yul evaluates expressions from right to left, so `returndatasize` will evaluate after `call`.
                     gt(returndatasize(), 0x1f),
                     call(
                         // If gas is the leftmost item before the call, it *should* be placed immediately before the
@@ -1020,6 +1021,8 @@ library ExecutionLib {
             // Perform the call
             success :=
                 and(
+                    // Yul evaluates expressions from right to left, so `returndatasize` will evaluate after
+                    // `staticcall`.
                     gt(returndatasize(), 0x1f),
                     staticcall(
                         gas(),

diff --git a/test/account/SigCallBuffer.t.sol b/test/account/SigCallBuffer.t.sol
@@ -21,14 +21,15 @@ import {ExecutionManifest} from "@erc6900/reference-implementation/interfaces/IE
 import {IValidationHookModule} from "@erc6900/reference-implementation/interfaces/IValidationHookModule.sol";
 import {IValidationModule} from "@erc6900/reference-implementation/interfaces/IValidationModule.sol";
 import {HookConfig, HookConfigLib} from "@erc6900/reference-implementation/libraries/HookConfigLib.sol";
-import {ModuleEntity} from "@erc6900/reference-implementation/libraries/ModuleEntityLib.sol";
+import {ModuleEntity, ModuleEntityLib} from "@erc6900/reference-implementation/libraries/ModuleEntityLib.sol";
 import {
     ValidationConfig,
     ValidationConfigLib
 } from "@erc6900/reference-implementation/libraries/ValidationConfigLib.sol";
 import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol";
 
 import {FALLBACK_VALIDATION} from "../../src/helpers/Constants.sol";
+import {ExecutionLib} from "../../src/libraries/ExecutionLib.sol";
 
 import {MockModule} from "../mocks/modules/MockModule.sol";
 import {AccountTestBase} from "../utils/AccountTestBase.sol";
@@ -137,6 +138,35 @@ contract SigCallBufferTest is AccountTestBase {
         );
     }
 
+    // Assert the call reverts if insufficient return data is provided for signature validation itself.
+    // This is only possible in non-SMA tests, as the signature validation function is done internally within SMA.
+    function test_sigCallBuffer_shortReturnData() public {
+        bytes32 hash = keccak256("test");
+
+        _setUp4ValidationHooks();
+
+        // Mock a call with <32 bytes of return data. This overrides an existing mock from the setup.
+        vm.mockCall(
+            address(validationModule),
+            abi.encodeWithSelector(IValidationModule.validateSignature.selector),
+            hex"abcdabcd"
+        );
+
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                ExecutionLib.SignatureValidationFunctionReverted.selector,
+                ModuleEntityLib.pack(address(validationModule), uint32(0)),
+                hex"abcdabcd"
+            )
+        );
+        vm.prank(beneficiary);
+        account1.isValidSignature(
+            hash, _encode1271Signature(_validationFunction, abi.encodePacked(EOA_TYPE_SIGNATURE))
+        );
+
+        vm.clearMockedCalls();
+    }
+
     function _setUp4ValidationHooks() internal {
         FuzzConfig memory fuzzConfig;
         fuzzConfig.validationHookCount = 4;

diff --git a/test/account/UOCallBuffer.t.sol b/test/account/UOCallBuffer.t.sol
@@ -218,7 +218,7 @@ contract UOCallBufferTest is AccountTestBase {
         account1.validateUserOp(userOp, userOpHash, 1 wei);
     }
 
-    function test_uoCallBuffer_shortReturnData() public withSMATest {
+    function test_uoCallBuffer_shortReturnData_preValidationHook() public withSMATest {
         _setup5ValidationHooks();
 
         PackedUserOperation memory userOp = PackedUserOperation({
@@ -255,6 +255,39 @@ contract UOCallBufferTest is AccountTestBase {
         vm.clearMockedCalls();
     }
 
+    function test_uoCallBuffer_shortReturnData_validationFunction() public {
+        PackedUserOperation memory userOp = PackedUserOperation({
+            sender: address(account1),
+            nonce: 0,
+            initCode: "",
+            callData: abi.encodeCall(account1.execute, (beneficiary, 0 wei, "")),
+            accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT),
+            preVerificationGas: 0,
+            gasFees: _encodeGas(1, 2),
+            paymasterAndData: "",
+            signature: _encodeSignature(_signerValidation, GLOBAL_VALIDATION, "abcdefghijklmnopqrstuvwxyz")
+        });
+        bytes32 userOpHash = entryPoint.getUserOpHash(userOp);
+
+        // mock a call to return less than 32 bytes of return data. This should cause validation to revert.
+
+        vm.mockCall(
+            address(singleSignerValidationModule),
+            abi.encodeWithSelector(IValidationModule.validateUserOp.selector),
+            hex"abcdabcd"
+        );
+
+        vm.prank(address(entryPoint));
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                ExecutionLib.UserOpValidationFunctionReverted.selector,
+                ModuleEntityLib.pack(address(singleSignerValidationModule), TEST_DEFAULT_VALIDATION_ENTITY_ID),
+                bytes(hex"abcdabcd")
+            )
+        );
+        account1.validateUserOp(userOp, userOpHash, 1 wei);
+    }
+
     function _setup5ValidationHooks() internal {
         ExecutionManifest memory m; // empty manifest