fix: [spearbit-88] move SIG_VALIDATION_X to a global var (#73)

Co-authored-by: Jay Paik <[email protected]>

src/account/UpgradeableModularAccount.sol

@@ -65,7 +65,6 @@ contract UpgradeableModularAccount is
     }
 
     IEntryPoint private immutable _ENTRY_POINT;
-    uint256 internal constant _SIG_VALIDATION_FAILED = 1;
 
     // As per the EIP-165 spec, no interface should ever match 0xffffffff
     bytes4 internal constant _INTERFACE_ID_INVALID = 0xffffffff;

src/helpers/ValidationDataHelpers.sol

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.22;
 
+import {SIG_VALIDATION_FAILED} from "../libraries/Constants.sol";
+
 /// @dev This helper function assumes that uint160(validationData1) and uint160(validationData2) can only be 0 or 1
 // solhint-disable-next-line private-vars-leading-underscore
 function _coalescePreValidation(uint256 validationData1, uint256 validationData2)
@@ -47,5 +49,6 @@ function _coalesceValidation(uint256 preValidationData, uint256 validationData)
     resValidationData |= ((validAfter1 < validAfter2) ? uint256(validAfter2) << 208 : uint256(validAfter1) << 208);
 
     // If prevalidation failed, bubble up failure
-    resValidationData |= uint160(preValidationData) == 1 ? 1 : uint160(validationData);
+    resValidationData |=
+        uint160(preValidationData) == SIG_VALIDATION_FAILED ? SIG_VALIDATION_FAILED : uint160(validationData);
 }

src/libraries/AssociatedLinkedListSetLib.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.22;
 
-import {SetValue, SENTINEL_VALUE, HAS_NEXT_FLAG} from "./LinkedListSetUtils.sol";
+import {SetValue, SENTINEL_VALUE, HAS_NEXT_FLAG} from "./Constants.sol";
 
 /// @dev Type representing the set, which is just a storage slot placeholder like the solidity mapping type.
 struct AssociatedLinkedListSet {

src/libraries/CastLib.sol

@@ -2,7 +2,7 @@
 pragma solidity ^0.8.22;
 
 import {FunctionReference} from "./FunctionReferenceLib.sol";
-import {SetValue} from "./LinkedListSetUtils.sol";
+import {SetValue} from "./Constants.sol";
 
 /// @title Cast Library
 /// @author Alchemy

src/libraries/LinkedListSetUtils.sol → src/libraries/Constants.sol

@@ -9,3 +9,7 @@ bytes32 constant SENTINEL_VALUE = bytes32(uint256(1));
 /// @dev Removing the last element will result in this flag not being set correctly, but all operations will
 /// function normally, albeit with one extra sload for getAll.
 bytes32 constant HAS_NEXT_FLAG = bytes32(uint256(2));
+
+/// @dev As defined by ERC-4337.
+uint256 constant SIG_VALIDATION_PASSED = 0;
+uint256 constant SIG_VALIDATION_FAILED = 1;

src/libraries/CountableLinkedListSetLib.sol

@@ -2,7 +2,7 @@
 pragma solidity ^0.8.22;
 
 import {LinkedListSet, LinkedListSetLib} from "./LinkedListSetLib.sol";
-import {SetValue} from "./LinkedListSetUtils.sol";
+import {SetValue} from "./Constants.sol";
 
 /// @title Countable Linked List Set Library
 /// @author Alchemy

src/libraries/LinkedListSetLib.sol

@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.22;
 
-import {SetValue, SENTINEL_VALUE, HAS_NEXT_FLAG} from "./LinkedListSetUtils.sol";
+import {SetValue, SENTINEL_VALUE, HAS_NEXT_FLAG} from "./Constants.sol";
 
 struct LinkedListSet {
     // Byte Layout

src/plugins/owner/MultiOwnerPlugin.sol

@@ -24,7 +24,7 @@ import {
     AssociatedLinkedListSet, AssociatedLinkedListSetLib
 } from "../../libraries/AssociatedLinkedListSetLib.sol";
 import {CastLib} from "../../libraries/CastLib.sol";
-import {SetValue} from "../../libraries/LinkedListSetUtils.sol";
+import {SetValue, SIG_VALIDATION_PASSED, SIG_VALIDATION_FAILED} from "../../libraries/Constants.sol";
 
 /// @title Multi Owner Plugin
 /// @author Alchemy
@@ -59,11 +59,6 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
     bytes32 private constant _HASHED_VERSION = keccak256(bytes(_VERSION));
     bytes32 private immutable _SALT = bytes32(bytes20(address(this)));
 
-    // ERC-4337 specific value: signature validation passed
-    uint256 internal constant _SIG_VALIDATION_PASSED = 0;
-    // ERC-4337 specific value: signature validation failed
-    uint256 internal constant _SIG_VALIDATION_FAILED = 1;
-
     // bytes4(keccak256("isValidSignature(bytes32,bytes)"))
     bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e;
     bytes4 internal constant _1271_MAGIC_VALUE_FAILURE = 0xffffffff;
@@ -183,14 +178,14 @@ contract MultiOwnerPlugin is BasePlugin, IMultiOwnerPlugin, IERC1271 {
             (address signer, ECDSA.RecoverError error) =
                 userOpHash.toEthSignedMessageHash().tryRecover(userOp.signature);
             if (error == ECDSA.RecoverError.NoError && isOwnerOf(msg.sender, signer)) {
-                return _SIG_VALIDATION_PASSED;
+                return SIG_VALIDATION_PASSED;
             }
 
             if (_isValidERC1271OwnerTypeSignature(msg.sender, userOpHash, userOp.signature)) {
-                return _SIG_VALIDATION_PASSED;
+                return SIG_VALIDATION_PASSED;
             }
 
-            return _SIG_VALIDATION_FAILED;
+            return SIG_VALIDATION_FAILED;
         }
 
         revert NotImplemented(msg.sig, functionId);

src/plugins/session/SessionKeyPlugin.sol

@@ -24,7 +24,9 @@ import {
     AssociatedLinkedListSet, AssociatedLinkedListSetLib
 } from "../../libraries/AssociatedLinkedListSetLib.sol";
 import {CastLib} from "../../libraries/CastLib.sol";
-import {SetValue, SENTINEL_VALUE} from "../../libraries/LinkedListSetUtils.sol";
+import {
+    SetValue, SENTINEL_VALUE, SIG_VALIDATION_PASSED, SIG_VALIDATION_FAILED
+} from "../../libraries/Constants.sol";
 
 /// @title Session Key Plugin
 /// @author Alchemy
@@ -42,9 +44,6 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi
     string internal constant _VERSION = "1.0.0";
     string internal constant _AUTHOR = "Alchemy";
 
-    uint256 internal constant _SIG_VALIDATION_PASSED = 0;
-    uint256 internal constant _SIG_VALIDATION_FAILED = 1;
-
     // Constants used in the manifest
     uint256 internal constant _MANIFEST_DEPENDENCY_INDEX_OWNER_USER_OP_VALIDATION = 0;
     uint256 internal constant _MANIFEST_DEPENDENCY_INDEX_OWNER_RUNTIME_VALIDATION = 1;
@@ -177,7 +176,7 @@ contract SessionKeyPlugin is ISessionKeyPlugin, SessionKeyPermissions, BasePlugi
                 ) {
                     return _checkUserOpPermissions(userOp, calls, sessionKey);
                 }
-                return _SIG_VALIDATION_FAILED;
+                return SIG_VALIDATION_FAILED;
             }
             revert InvalidSignature(sessionKey);
         }

src/plugins/session/permissions/SessionKeyPermissions.sol

@@ -10,6 +10,7 @@ import {SessionKeyPermissionsLoupe} from "./SessionKeyPermissionsLoupe.sol";
 
 import {IStandardExecutor} from "../../../interfaces/IStandardExecutor.sol";
 import {UserOperation} from "../../../interfaces/erc4337/UserOperation.sol";
+import {SIG_VALIDATION_PASSED, SIG_VALIDATION_FAILED} from "../../../libraries/Constants.sol";
 
 /// @title Session Key Permissions
 /// @author Alchemy
@@ -128,8 +129,8 @@ abstract contract SessionKeyPermissions is ISessionKeyPlugin, SessionKeyPermissi
         // otherwise a packed struct of the aggregator address (0 here), and two
         // 6-byte timestamps indicating the start and end times at which the op
         // is valid.
-        return uint160(validationSuccess ? 0 : 1) | (uint256(validUntil) << 160)
-            | (uint256(currentValidAfter) << (208));
+        return uint160(validationSuccess ? SIG_VALIDATION_PASSED : SIG_VALIDATION_FAILED)
+            | (uint256(validUntil) << 160) | (uint256(currentValidAfter) << (208));
     }
 
     /// @dev Checks permissions on a per-call basis. Should be run during user op validation once per `Call` struct

test/account/ValidationIntersection.t.sol

@@ -9,6 +9,7 @@ import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAcc
 import {IEntryPoint} from "../../src/interfaces/erc4337/IEntryPoint.sol";
 import {UserOperation} from "../../src/interfaces/erc4337/UserOperation.sol";
 import {FunctionReference} from "../../src/interfaces/IPluginManager.sol";
+import {SIG_VALIDATION_FAILED, SIG_VALIDATION_PASSED} from "../../src/libraries/Constants.sol";
 import {MultiOwnerPlugin} from "../../src/plugins/owner/MultiOwnerPlugin.sol";
 
 import {MultiOwnerMSCAFactory} from "../../src/factory/MultiOwnerMSCAFactory.sol";
@@ -20,8 +21,6 @@ import {
 } from "../mocks/plugins/ValidationPluginMocks.sol";
 
 contract ValidationIntersectionTest is Test {
-    uint256 internal constant _SIG_VALIDATION_FAILED = 1;
-
     IEntryPoint public entryPoint;
 
     address public owner1;
@@ -92,8 +91,8 @@ contract ValidationIntersectionTest is Test {
 
     function test_validationIntersect_authorizer_sigfail_validationFunction() public {
         oneHookPlugin.setValidationData(
-            _SIG_VALIDATION_FAILED,
-            0 // returns OK
+            SIG_VALIDATION_FAILED,
+            SIG_VALIDATION_PASSED // returns OK
         );
 
         UserOperation memory userOp;
@@ -104,13 +103,13 @@ contract ValidationIntersectionTest is Test {
         uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei);
 
         // Down-cast to only check the authorizer
-        assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED);
+        assertEq(uint160(returnedValidationData), SIG_VALIDATION_FAILED);
     }
 
     function test_validationIntersect_authorizer_sigfail_hook() public {
         oneHookPlugin.setValidationData(
-            0, // returns OK
-            _SIG_VALIDATION_FAILED
+            SIG_VALIDATION_PASSED, // returns OK
+            SIG_VALIDATION_FAILED
         );
 
         UserOperation memory userOp;
@@ -121,7 +120,7 @@ contract ValidationIntersectionTest is Test {
         uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei);
 
         // Down-cast to only check the authorizer
-        assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED);
+        assertEq(uint160(returnedValidationData), SIG_VALIDATION_FAILED);
     }
 
     function test_validationIntersect_timeBounds_intersect_1() public {
@@ -170,7 +169,7 @@ contract ValidationIntersectionTest is Test {
         address badAuthorizer = makeAddr("badAuthorizer");
 
         oneHookPlugin.setValidationData(
-            0, // returns OK
+            SIG_VALIDATION_PASSED, // returns OK
             uint256(uint160(badAuthorizer)) // returns an aggregator, which preValidation hooks are not allowed to
                 // do.
         );
@@ -196,7 +195,7 @@ contract ValidationIntersectionTest is Test {
 
         oneHookPlugin.setValidationData(
             uint256(uint160(goodAuthorizer)), // returns a valid aggregator
-            0 // returns OK
+            SIG_VALIDATION_PASSED // returns OK
         );
 
         UserOperation memory userOp;
@@ -240,7 +239,7 @@ contract ValidationIntersectionTest is Test {
         uint48 end2 = uint48(25);
 
         twoHookPlugin.setValidationData(
-            0, // returns OK
+            SIG_VALIDATION_PASSED, // returns OK
             _packValidationData(address(0), start1, end1),
             _packValidationData(address(0), start2, end2)
         );
@@ -257,9 +256,9 @@ contract ValidationIntersectionTest is Test {
 
     function test_validationIntersect_multiplePreValidationHooksSigFail() public {
         twoHookPlugin.setValidationData(
-            0, // returns OK
-            0, // returns OK
-            _SIG_VALIDATION_FAILED
+            SIG_VALIDATION_PASSED, // returns OK
+            SIG_VALIDATION_PASSED, // returns OK
+            SIG_VALIDATION_FAILED
         );
 
         UserOperation memory userOp;
@@ -271,7 +270,7 @@ contract ValidationIntersectionTest is Test {
         uint256 returnedValidationData = account1.validateUserOp(userOp, uoHash, 1 wei);
 
         // Down-cast to only check the authorizer
-        assertEq(uint160(returnedValidationData), _SIG_VALIDATION_FAILED);
+        assertEq(uint160(returnedValidationData), SIG_VALIDATION_FAILED);
     }
 
     function _unpackValidationData(uint256 validationData)

test/invariant/handlers/AssociatedLinkedListSetHandler.sol

@@ -11,7 +11,7 @@ import {
     AssociatedLinkedListSet,
     AssociatedLinkedListSetLib
 } from "../../../src/libraries/AssociatedLinkedListSetLib.sol";
-import {SetValue} from "../../../src/libraries/LinkedListSetUtils.sol";
+import {SetValue} from "../../../src/libraries/Constants.sol";
 
 /// @notice A handler contract for differential invariant testing AssociatedLinkedListSetLib
 ///         This contract maps logic for adding, removeing, clearing, and inspecting a list

test/invariant/handlers/LinkedListSetHandler.sol

@@ -8,7 +8,7 @@ import {StdUtils} from "forge-std/StdUtils.sol";
 import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol";
 import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol";
 import {LinkedListSetLib, LinkedListSet as EnumerableSetType} from "../../../src/libraries/LinkedListSetLib.sol";
-import {SetValue} from "../../../src/libraries/LinkedListSetUtils.sol";
+import {SetValue} from "../../../src/libraries/Constants.sol";
 
 /// @notice A handler contract for differential invariant testing LinkedListSetLib
 ///         This contract maps logic for adding, removeing, clearing, and inspecting a list

test/libraries/AssociatedLinkedListSetLib.t.sol

@@ -7,7 +7,7 @@ import {
     AssociatedLinkedListSet,
     AssociatedLinkedListSetLib
 } from "../../src/libraries/AssociatedLinkedListSetLib.sol";
-import {SetValue, SENTINEL_VALUE} from "../../src/libraries/LinkedListSetUtils.sol";
+import {SetValue, SENTINEL_VALUE} from "../../src/libraries/Constants.sol";
 
 contract AssociatedLinkedListSetLibTest is Test {
     using AssociatedLinkedListSetLib for AssociatedLinkedListSet;

test/libraries/CountableLinkedListSetLib.t.sol

@@ -5,7 +5,7 @@ import {Test} from "forge-std/Test.sol";
 
 import {CountableLinkedListSetLib} from "../../src/libraries/CountableLinkedListSetLib.sol";
 import {LinkedListSet, LinkedListSetLib} from "../../src/libraries/LinkedListSetLib.sol";
-import {SetValue} from "../../src/libraries/LinkedListSetUtils.sol";
+import {SetValue} from "../../src/libraries/Constants.sol";
 
 contract CountableLinkedListSetLibTest is Test {
     using LinkedListSetLib for LinkedListSet;

test/libraries/LinkedListSetLib.t.sol

@@ -4,7 +4,7 @@ pragma solidity ^0.8.22;
 import {Test} from "forge-std/Test.sol";
 
 import {LinkedListSet, LinkedListSetLib} from "../../src/libraries/LinkedListSetLib.sol";
-import {SetValue, SENTINEL_VALUE} from "../../src/libraries/LinkedListSetUtils.sol";
+import {SetValue, SENTINEL_VALUE} from "../../src/libraries/Constants.sol";
 
 // Ported over from test/AssociatedLinkedListSetLib.t.sol, dropping test_no_address_collision
 contract LinkedListSetLibTest is Test {