Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jan 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/account/UpgradeableModularAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 4 additions & 1 deletion src/helpers/ValidationDataHelpers.sol
Original file line number Diff line number Diff line change
@@ -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)
Expand Down Expand Up @@ -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);
}
2 changes: 1 addition & 1 deletion src/libraries/AssociatedLinkedListSetLib.sol
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/CastLib.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
jaypaik marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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;
2 changes: 1 addition & 1 deletion src/libraries/CountableLinkedListSetLib.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/LinkedListSetLib.sol
Original file line number Diff line number Diff line change
@@ -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
Expand Down
13 changes: 4 additions & 9 deletions src/plugins/owner/MultiOwnerPlugin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
9 changes: 4 additions & 5 deletions src/plugins/session/SessionKeyPlugin.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

@0xrubes 0xrubes Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import of SIG_VALIDATION_PASSED, else lgtm

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch! addressing this in: #113

} from "../../libraries/Constants.sol";

/// @title Session Key Plugin
/// @author Alchemy
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
5 changes: 3 additions & 2 deletions src/plugins/session/permissions/SessionKeyPermissions.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
29 changes: 14 additions & 15 deletions test/account/ValidationIntersection.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
);
Expand All @@ -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;
Expand Down Expand Up @@ -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)
);
Expand All @@ -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;
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion test/invariant/handlers/AssociatedLinkedListSetHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/invariant/handlers/LinkedListSetHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/AssociatedLinkedListSetLib.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/CountableLinkedListSetLib.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion test/libraries/LinkedListSetLib.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down