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

100% Code Coverage on Signer Launchpad #411

Merged
merged 2 commits into from
May 13, 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
36 changes: 18 additions & 18 deletions modules/passkey/contracts/4337/SafeSignerLaunchpad.sol
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,15 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
* @notice The EIP-712 type-hash for the domain separator used for verifying Safe initialization signatures.
* @custom:computed-as keccak256("EIP712Domain(uint256 chainId,address verifyingContract)")
*/
bytes32 private constant DOMAIN_SEPARATOR_TYPEHASH = 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218;
bytes32 private constant _DOMAIN_SEPARATOR_TYPEHASH = 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Consistent naming.


/**
* @notice The storage slot used for the target Safe singleton address to promote to.
* @custom:computed-as keccak256("SafeSignerLaunchpad.targetSingleton") - 1
* @dev This value is intentionally computed to be a hash -1 as a precaution to avoid any potential issues from
* unintended hash collisions.
*/
uint256 private constant TARGET_SINGLETON_SLOT = 0x610b07c5cf4b478e92ab041de73a412736c750f1bf07a613600b24b3a8bd597e;
uint256 private constant _TARGET_SINGLETON_SLOT = 0x610b07c5cf4b478e92ab041de73a412736c750f1bf07a613600b24b3a8bd597e;

/**
* @notice The keccak256 hash of the EIP-712 SafeInitOp struct, representing the user operation to execute alongside initialization.
Expand All @@ -43,7 +43,7 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
* {address} entryPoint - The address of the entry point that will execute the user operation.
* @custom:computed-as keccak256("SafeInitOp(bytes32 userOpHash,uint48 validAfter,uint48 validUntil,address entryPoint)")
*/
bytes32 private constant SAFE_INIT_OP_TYPEHASH = 0x25838d3914a61e3531f21f12b8cd3110a5f9d478292d07dd197859a5c4eaacb2;
bytes32 private constant _SAFE_INIT_OP_TYPEHASH = 0x25838d3914a61e3531f21f12b8cd3110a5f9d478292d07dd197859a5c4eaacb2;

/**
* @notice An error indicating that the entry point used when deploying a new module instance is invalid.
Expand All @@ -67,10 +67,9 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
error AlreadyInitialized();

/**
* @notice An error indicating an attempt to execute a user operation on an account that has already been promoted
* to a Safe singleton.
* @notice An error indicating an attempt to execute a user operation on an account that has not been initialized.
*/
error AlreadyPromoted();
error NotInitialized();

/**
* @notice An error indicating that the account was initialized with an invalid Safe singleton address.
Expand Down Expand Up @@ -201,6 +200,14 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
}
}

/**
* @notice Computes the EIP-712 domain separator for Safe launchpad operations.
* @return domainSeparatorHash The EIP-712 domain separator hash for this contract.
*/
function domainSeparator() public view returns (bytes32) {
return keccak256(abi.encode(_DOMAIN_SEPARATOR_TYPEHASH, block.chainid, _SELF));
}

/**
* @notice Compute the {SafeInitOp} hash of the first user operation that initializes the Safe.
* @dev The hash is generated using the keccak256 hash function and the EIP-712 standard. It is signed by the Safe
Expand All @@ -216,7 +223,7 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
abi.encodePacked(
bytes2(0x1901),
domainSeparator(),
keccak256(abi.encode(SAFE_INIT_OP_TYPEHASH, userOpHash, validAfter, validUntil, SUPPORTED_ENTRYPOINT))
keccak256(abi.encode(_SAFE_INIT_OP_TYPEHASH, userOpHash, validAfter, validUntil, SUPPORTED_ENTRYPOINT))
)
);
}
Expand Down Expand Up @@ -280,7 +287,7 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
) external onlySupportedEntryPoint {
address singleton = _targetSingleton();
if (singleton == address(0)) {
revert AlreadyPromoted();
revert NotInitialized();
}

SafeStorage.singleton = singleton;
Expand All @@ -303,6 +310,7 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
// See <https://docs.soliditylang.org/en/latest/control-structures.html#panic-via-assert-and-error-via-require>.
mstore(0x00, hex"4e487b71")
mstore(0x04, 0x21)
revert(0, 0x24)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bug caught by the unit tests.

}
}

Expand All @@ -311,14 +319,6 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
}
}

/**
* @notice Computes the EIP-712 domain separator for Safe launchpad operations.
* @return domainSeparatorHash The EIP-712 domain separator hash for this contract.
*/
function domainSeparator() public view returns (bytes32) {
return keccak256(abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, _SELF));
}

/**
* @dev Validates that the user operation is correctly signed and returns an ERC-4337 packed validation data
* of `validAfter || validUntil || authorizer`:
Expand Down Expand Up @@ -378,7 +378,7 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
assembly ("memory-safe") {
// Note that we explicitly don't mask the address, as Solidity will generate masking code for every time
// the variable is read.
value := sload(TARGET_SINGLETON_SLOT)
value := sload(_TARGET_SINGLETON_SLOT)
}
}

Expand All @@ -389,7 +389,7 @@ contract SafeSignerLaunchpad is IAccount, SafeStorage {
function _setTargetSingleton(address value) internal {
// solhint-disable-next-line no-inline-assembly
assembly ("memory-safe") {
sstore(TARGET_SINGLETON_SLOT, value)
sstore(_TARGET_SINGLETON_SLOT, value)
}
}
}
1 change: 1 addition & 0 deletions modules/passkey/contracts/test/TestDependencies.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ import "@account-abstraction/contracts/samples/VerifyingPaymaster.sol";
import "@safe-global/mock-contract/contracts/MockContract.sol";
import "@safe-global/safe-4337/contracts/test/TestStakedFactory.sol";
import "@safe-global/safe-contracts/contracts/proxies/SafeProxyFactory.sol";
import "@safe-global/safe-contracts/contracts/libraries/MultiSend.sol";
Loading
Loading