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

🔒 M-05 - Revert on Validator Not Installed in validateUserOp() #121

Merged
merged 21 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
482e6cc
🔒️ h01 fix by adding msg.value
Aboudjem Jul 29, 2024
c10eb71
fix: handle inner call failure and msgvalue in factory contracts
Aboudjem Jul 29, 2024
c264761
refactor Bootstrap contract to improve code organization
Aboudjem Jul 29, 2024
692a1db
feat: Add validation for 's' value in K1Validator
Aboudjem Jul 30, 2024
1d4cbb8
feat: Add tests for isValidSignatureWithSender and transferOwnership …
Aboudjem Jul 30, 2024
5a75c21
feat: Add validation for 's' value in K1Validator
Aboudjem Jul 30, 2024
60b93a9
feat: Add validation for 's' value in K1Validator
Aboudjem Jul 30, 2024
ef5c7b6
feat: Add Storage contract and update BaseAccount to use it
Aboudjem Jul 30, 2024
303369f
add new modifier onlyAuthorized
Aboudjem Jul 30, 2024
6f94787
feat: fix account creation test in TestK1ValidatorFactory_Deployments
Aboudjem Jul 30, 2024
5a9eacf
feat: Add _ENTRYPOINT address to Storage contract
Aboudjem Jul 30, 2024
6ac9161
refactor: Update fallback function tests in TextNexus_FallbackFunctio…
Aboudjem Jul 30, 2024
3f48ffb
feat: Improve validation for 's' value in K1Validator
Aboudjem Jul 30, 2024
95a26dc
revert when invalid validation module on validateuserop to comply wit…
Aboudjem Jul 31, 2024
ec8d6bb
refactor: Update TestERC4337Account_ValidateUserOp.t.sol to use rever…
Aboudjem Jul 31, 2024
12ea0f2
refactor: Update TestFuzz_ERC4337Account.t.sol to use revert instead …
Aboudjem Jul 31, 2024
429f01f
refactor: Update TestFuzz_ValidateUserOp.t.sol to use revert instead …
Aboudjem Jul 31, 2024
684a39c
format fix
Aboudjem Jul 31, 2024
d7f9219
Merge branch 'remediations/cantina-spearbit' into fix/security-m-05
livingrockrises Aug 19, 2024
7f5050a
refactor
livingrockrises Aug 20, 2024
cd4d8de
fix foundry tests
livingrockrises Aug 20, 2024
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
96 changes: 41 additions & 55 deletions contracts/Nexus.sol
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,19 @@ import { IERC7484 } from "./interfaces/IERC7484.sol";
import { ModuleManager } from "./base/ModuleManager.sol";
import { ExecutionHelper } from "./base/ExecutionHelper.sol";
import { IValidator } from "./interfaces/modules/IValidator.sol";
import {
MODULE_TYPE_VALIDATOR,
MODULE_TYPE_EXECUTOR,
MODULE_TYPE_FALLBACK,
MODULE_TYPE_HOOK,
MODULE_TYPE_MULTI,
VALIDATION_FAILED
import {
MODULE_TYPE_VALIDATOR, MODULE_TYPE_EXECUTOR, MODULE_TYPE_FALLBACK, MODULE_TYPE_HOOK, MODULE_TYPE_MULTI, VALIDATION_FAILED
} from "./types/Constants.sol";
import {
ModeLib,
ExecutionMode,
ExecType,
CallType,
CALLTYPE_BATCH,
CALLTYPE_SINGLE,
CALLTYPE_DELEGATECALL,
EXECTYPE_DEFAULT,
EXECTYPE_TRY
import {
ModeLib,
ExecutionMode,
ExecType,
CallType,
CALLTYPE_BATCH,
CALLTYPE_SINGLE,
CALLTYPE_DELEGATECALL,
EXECTYPE_DEFAULT,
EXECTYPE_TRY
} from "./lib/ModeLib.sol";
import { NonceLib } from "./lib/NonceLib.sol";

Expand Down Expand Up @@ -96,16 +91,21 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
PackedUserOperation calldata op,
bytes32 userOpHash,
uint256 missingAccountFunds
) external virtual payPrefund(missingAccountFunds) onlyEntryPoint returns (uint256 validationData) {
)
external
virtual
payPrefund(missingAccountFunds)
onlyEntryPoint
returns (uint256 validationData)
{
address validator = op.nonce.getValidator();
if (op.nonce.isModuleEnableMode()) {
PackedUserOperation memory userOp = op;
userOp.signature = _enableMode(userOpHash, op.signature);
if (!_isValidatorInstalled(validator)) return VALIDATION_FAILED;
require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));
validationData = IValidator(validator).validateUserOp(userOp, userOpHash);
} else {
// Check if validator is not enabled. If not, return VALIDATION_FAILED.
if (!_isValidatorInstalled(validator)) return VALIDATION_FAILED;
require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));
validationData = IValidator(validator).validateUserOp(op, userOpHash);
}
}
Expand Down Expand Up @@ -136,7 +136,14 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
function executeFromExecutor(
ExecutionMode mode,
bytes calldata executionCalldata
) external payable onlyExecutorModule withHook withRegistry(msg.sender, MODULE_TYPE_EXECUTOR) returns (bytes[] memory returnData) {
)
external
payable
onlyExecutorModule
withHook
withRegistry(msg.sender, MODULE_TYPE_EXECUTOR)
returns (bytes[] memory returnData)
{
(CallType callType, ExecType execType) = mode.decodeBasic();
// check if calltype is batch or single or delegate call
if (callType == CALLTYPE_SINGLE) {
Expand Down Expand Up @@ -165,7 +172,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
(address target, bytes memory data) = abi.decode(innerCall, (address, bytes));
bool success;
// Perform the call to the target contract with the decoded data.
(success, innerCallRet) = target.call(data);
(success, innerCallRet) = target.call{ value: msg.value }(data);
// Ensure the call was successful.
require(success, InnerCallFailed());
}
Expand Down Expand Up @@ -218,7 +225,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
(bool success, ) = bootstrap.delegatecall(bootstrapCall);

require(success, NexusInitializationFailed());
require(_hasValidators(), MissingValidator());
require(_hasValidators(), NoValidatorInstalled());
}

function setRegistry(IERC7484 newRegistry, address[] calldata attesters, uint8 threshold) external payable onlyEntryPointOrSelf {
Expand All @@ -234,7 +241,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
function isValidSignature(bytes32 hash, bytes calldata data) external view virtual override returns (bytes4) {
// First 20 bytes of data will be validator address and rest of the bytes is complete signature.
address validator = address(bytes20(data[0:20]));
require(_isValidatorInstalled(validator), InvalidModule(validator));
require(_isValidatorInstalled(validator), ValidatorNotInstalled(validator));
(bytes32 computeHash, bytes calldata truncatedSignature) = _erc1271HashForIsValidSignatureViaNestedEIP712(hash, data[20:]);
return IValidator(validator).isValidSignatureWithSender(msg.sender, computeHash, truncatedSignature);
}
Expand Down Expand Up @@ -272,9 +279,8 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
(CallType callType, ExecType execType) = mode.decodeBasic();

// Return true if both the call type and execution type are supported.
return
(callType == CALLTYPE_SINGLE || callType == CALLTYPE_BATCH || callType == CALLTYPE_DELEGATECALL) &&
(execType == EXECTYPE_DEFAULT || execType == EXECTYPE_TRY);
return (callType == CALLTYPE_SINGLE || callType == CALLTYPE_BATCH || callType == CALLTYPE_DELEGATECALL)
&& (execType == EXECTYPE_DEFAULT || execType == EXECTYPE_TRY);
}

/// @notice Determines whether a module is installed on the smart account.
Expand Down Expand Up @@ -352,7 +358,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
/// @dev Ensures that only authorized callers can upgrade the smart contract implementation.
/// This is part of the UUPS (Universal Upgradeable Proxy Standard) pattern.
/// @param newImplementation The address of the new implementation to upgrade to.
function _authorizeUpgrade(address newImplementation) internal virtual override(UUPSUpgradeable) onlyEntryPointOrSelf {}
function _authorizeUpgrade(address newImplementation) internal virtual override(UUPSUpgradeable) onlyEntryPointOrSelf { }

/// @notice Returns the EIP-712 typed hash of the `BiconomyNexusMessage(bytes32 hash)` data structure.
///
Expand Down Expand Up @@ -435,10 +441,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
/// you can choose a more minimalistic signature scheme like
/// `keccak256(abi.encode(address(this), hash))` instead of all these acrobatics.
/// All these are just for widespread out-of-the-box compatibility with other wallet clients.
function _erc1271HashForIsValidSignatureViaNestedEIP712(
bytes32 hash,
bytes calldata signature
) internal view virtual returns (bytes32, bytes calldata) {
function _erc1271HashForIsValidSignatureViaNestedEIP712(bytes32 hash, bytes calldata signature) internal view virtual returns (bytes32, bytes calldata) {
assembly {
// Unwraps the ERC6492 wrapper if it exists.
// See: https://eips.ethereum.org/EIPS/eip-6492
Expand All @@ -459,11 +462,7 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
let m := mload(0x40) // Cache the free memory pointer.
// Length of the contents type.
let c := and(0xffff, calldataload(add(signature.offset, sub(signature.length, 0x20))))
for {

} 1 {

} {
for { } 1 { } {
let l := add(0x42, c) // Total length of appended data (32 + 32 + c + 2).
let o := add(signature.offset, sub(signature.length, l))
calldatacopy(0x20, o, 0x40) // Copy the `APP_DOMAIN_SEPARATOR` and contents struct hash.
Expand All @@ -483,15 +482,9 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra
let d := byte(0, mload(p)) // For denoting if the contents name is invalid.
d := or(gt(26, sub(d, 97)), eq(40, d)) // Starts with lowercase or '('.
// Store the end sentinel '(', and advance `p` until we encounter a '(' byte.
for {
mstore(add(p, c), 40)
} 1 {
p := add(p, 1)
} {
for { mstore(add(p, c), 40) } 1 { p := add(p, 1) } {
let b := byte(0, mload(p))
if eq(40, b) {
break
}
if eq(40, b) { break }
d := or(d, shr(b, 0x120100000001)) // Has a byte in ", )\x00".
}
mstore(p, " contents,bytes1 fields,string n")
Expand Down Expand Up @@ -525,15 +518,8 @@ contract Nexus is INexus, BaseAccount, ExecutionHelper, ModuleManager, UUPSUpgra

/// @dev For use in `_erc1271HashForIsValidSignatureViaNestedEIP712`,
function _typedDataSignFields() private view returns (bytes32 m) {
(
bytes1 fields,
string memory name,
string memory version,
uint256 chainId,
address verifyingContract,
bytes32 salt,
uint256[] memory extensions
) = eip712Domain();
(bytes1 fields, string memory name, string memory version, uint256 chainId, address verifyingContract, bytes32 salt, uint256[] memory extensions) =
eip712Domain();
/// @solidity memory-safe-assembly
assembly {
m := mload(0x40) // Grab the free memory pointer.
Expand Down
29 changes: 13 additions & 16 deletions contracts/base/BaseAccount.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ pragma solidity ^0.8.26;
// Learn more at https://biconomy.io. To report security issues, please contact us at: security@biconomy.io

import { IEntryPoint } from "account-abstraction/contracts/interfaces/IEntryPoint.sol";

import { Storage } from "./Storage.sol";
import { IBaseAccount } from "../interfaces/base/IBaseAccount.sol";

/// @title Nexus - BaseAccount
Expand All @@ -23,14 +25,10 @@ import { IBaseAccount } from "../interfaces/base/IBaseAccount.sol";
/// @author @filmakarov | Biconomy | filipp.makarov@biconomy.io
/// @author @zeroknots | Rhinestone.wtf | zeroknots.eth
/// Special thanks to the Solady team for foundational contributions: https://github.com/Vectorized/solady
contract BaseAccount is IBaseAccount {
contract BaseAccount is Storage, IBaseAccount {
/// @notice Identifier for this implementation on the network
string internal constant _ACCOUNT_IMPLEMENTATION_ID = "biconomy.nexus.1.0.0-beta";

/// @notice The canonical address for the ERC4337 EntryPoint contract, version 0.7.
/// This address is consistent across all supported networks.
address internal immutable _ENTRYPOINT;

/// @dev Ensures the caller is either the EntryPoint or this account itself.
/// Reverts with AccountAccessUnauthorized if the check fails.
modifier onlyEntryPointOrSelf() {
Expand Down Expand Up @@ -69,9 +67,7 @@ contract BaseAccount is IBaseAccount {
/// @solidity memory-safe-assembly
assembly {
// The EntryPoint has balance accounting logic in the `receive()` function.
if iszero(call(gas(), entryPointAddress, callvalue(), codesize(), 0x00, codesize(), 0x00)) {
revert(codesize(), 0x00)
} // For gas estimation.
if iszero(call(gas(), entryPointAddress, callvalue(), codesize(), 0x00, codesize(), 0x00)) { revert(codesize(), 0x00) } // For gas estimation.
}
}

Expand Down Expand Up @@ -108,15 +104,16 @@ contract BaseAccount is IBaseAccount {
assembly {
mstore(0x20, address()) // Store the `account` argument.
mstore(0x00, 0x70a08231) // `balanceOf(address)`.
result := mul(
// Returns 0 if the EntryPoint does not exist.
mload(0x20),
and(
// The arguments of `and` are evaluated from right to left.
gt(returndatasize(), 0x1f), // At least 32 bytes returned.
staticcall(gas(), entryPointAddress, 0x1c, 0x24, 0x20, 0x20)
result :=
mul(
// Returns 0 if the EntryPoint does not exist.
mload(0x20),
and(
// The arguments of `and` are evaluated from right to left.
gt(returndatasize(), 0x1f), // At least 32 bytes returned.
staticcall(gas(), entryPointAddress, 0x1c, 0x24, 0x20, 0x20)
)
)
)
}
}

Expand Down
Loading
Loading