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.

Sign up for GitHub

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/excess override on is valid signature unsafe #193

Closed
wants to merge 4 commits into from
Closed

Fix/excess override on is valid signature unsafe #193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c976667
isValidSignature remove override
Jan 19, 2024
3cfe97a
fix excess override, add virtual
Jan 19, 2024
00608e4
remove excess override
Jan 19, 2024
2a24bf0
excess overrides removed from modules
Jan 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
4 changes: 2 additions & 2 deletions contracts/smart-account/modules/AccountRecoveryModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ contract AccountRecoveryModule is
function isValidSignature(
bytes32,
bytes memory
) public view virtual override returns (bytes4) {
) public view virtual returns (bytes4) {
return 0xffffffff; // not supported
}

Expand All @@ -499,7 +499,7 @@ contract AccountRecoveryModule is
function isValidSignatureUnsafe(
bytes32,
bytes memory
) public view virtual override returns (bytes4) {
) public view virtual returns (bytes4) {
return 0xffffffff; // not supported
}

Expand Down
6 changes: 3 additions & 3 deletions contracts/smart-account/modules/BatchedSessionRouterModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ contract BatchedSessionRouter is
function validateUserOp(
UserOperation calldata userOp,
bytes32 userOpHash
) external virtual override returns (uint256) {
) external virtual returns (uint256) {
{
// check this is a proper method call
bytes4 selector = bytes4(userOp.callData[0:4]);
Expand Down Expand Up @@ -151,7 +151,7 @@ contract BatchedSessionRouter is
function isValidSignature(
bytes32 _dataHash,
bytes memory _signature
) public pure override returns (bytes4) {
) public pure virtual returns (bytes4) {
(_dataHash, _signature);
return 0xffffffff; // do not support it here
}
Expand All @@ -162,7 +162,7 @@ contract BatchedSessionRouter is
function isValidSignatureUnsafe(
bytes32,
bytes memory
) public pure virtual override returns (bytes4) {
) public pure virtual returns (bytes4) {
return 0xffffffff; // not supported
}
}
18 changes: 7 additions & 11 deletions contracts/smart-account/modules/EcdsaOwnershipRegistryModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,7 @@ contract EcdsaOwnershipRegistryModule is
mapping(address => address) internal _smartAccountOwners;

/// @inheritdoc IEcdsaOwnershipRegistryModule
function initForSmartAccount(
address eoaOwner
) external override returns (address) {
function initForSmartAccount(address eoaOwner) external returns (address) {
if (_smartAccountOwners[msg.sender] != address(0)) {
revert AlreadyInitedForSmartAccount(msg.sender);
}
Expand All @@ -49,21 +47,19 @@ contract EcdsaOwnershipRegistryModule is
}

/// @inheritdoc IEcdsaOwnershipRegistryModule
function transferOwnership(address owner) external override {
function transferOwnership(address owner) external {
if (_isSmartContract(owner)) revert NotEOA(owner);
if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
_transferOwnership(msg.sender, owner);
}

/// @inheritdoc IEcdsaOwnershipRegistryModule
function renounceOwnership() external override {
function renounceOwnership() external {
_transferOwnership(msg.sender, address(0));
}

/// @inheritdoc IEcdsaOwnershipRegistryModule
function getOwner(
address smartAccount
) external view override returns (address) {
function getOwner(address smartAccount) external view returns (address) {
address owner = _smartAccountOwners[smartAccount];
if (owner == address(0)) {
revert NoOwnerRegisteredForSmartAccount(smartAccount);
Expand All @@ -75,7 +71,7 @@ contract EcdsaOwnershipRegistryModule is
function validateUserOp(
UserOperation calldata userOp,
bytes32 userOpHash
) external view virtual override returns (uint256) {
) external view virtual returns (uint256) {
if (
_verifySignature(
userOpHash,
Expand All @@ -101,7 +97,7 @@ contract EcdsaOwnershipRegistryModule is
function isValidSignature(
bytes32 dataHash,
bytes memory moduleSignature
) public view virtual override returns (bytes4) {
) public view virtual returns (bytes4) {
return
isValidSignatureForAddress(dataHash, moduleSignature, msg.sender);
}
Expand All @@ -111,7 +107,7 @@ contract EcdsaOwnershipRegistryModule is
bytes32 dataHash,
bytes memory moduleSignature,
address smartAccount
) public view virtual override returns (bytes4) {
) public view virtual returns (bytes4) {
if (
_verifySignature(
keccak256(
Expand Down
17 changes: 7 additions & 10 deletions contracts/smart-account/modules/MultiOwnedECDSAModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,7 @@ contract MultiOwnedECDSAModule is
}

/// @inheritdoc IMultiOwnedECDSAModule
function transferOwnership(
address owner,
address newOwner
) external override {
function transferOwnership(address owner, address newOwner) external {
if (_isSmartContract(newOwner)) revert NotEOA(newOwner);
if (newOwner == address(0)) revert ZeroAddressNotAllowedAsOwner();
if (owner == newOwner)
Expand All @@ -79,7 +76,7 @@ contract MultiOwnedECDSAModule is
}

/// @inheritdoc IMultiOwnedECDSAModule
function addOwner(address owner) external override {
function addOwner(address owner) external {
if (_isSmartContract(owner)) revert NotEOA(owner);
if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
if (_smartAccountOwners[owner][msg.sender])
Expand All @@ -93,7 +90,7 @@ contract MultiOwnedECDSAModule is
}

/// @inheritdoc IMultiOwnedECDSAModule
function removeOwner(address owner) external override {
function removeOwner(address owner) external {
if (!_smartAccountOwners[owner][msg.sender])
revert NotAnOwner(owner, msg.sender);
_smartAccountOwners[owner][msg.sender] = false;
Expand All @@ -107,15 +104,15 @@ contract MultiOwnedECDSAModule is
function isOwner(
address smartAccount,
address eoaAddress
) external view override returns (bool) {
) external view returns (bool) {
return _smartAccountOwners[eoaAddress][smartAccount];
}

/// @inheritdoc IAuthorizationModule
function validateUserOp(
UserOperation calldata userOp,
bytes32 userOpHash
) external view virtual override returns (uint256) {
) external view virtual returns (uint256) {
(bytes memory cleanEcdsaSignature, ) = abi.decode(
userOp.signature,
(bytes, address)
Expand All @@ -137,7 +134,7 @@ contract MultiOwnedECDSAModule is
function isValidSignature(
bytes32 dataHash,
bytes memory moduleSignature
) public view virtual override returns (bytes4) {
) public view virtual returns (bytes4) {
return
isValidSignatureForAddress(dataHash, moduleSignature, msg.sender);
}
Expand All @@ -147,7 +144,7 @@ contract MultiOwnedECDSAModule is
bytes32 dataHash,
bytes memory moduleSignature,
address smartAccount
) public view virtual override returns (bytes4) {
) public view virtual returns (bytes4) {
if (
_verifySignature(
keccak256(
Expand Down
4 changes: 2 additions & 2 deletions contracts/smart-account/modules/PasskeyRegistryModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ contract PasskeyRegistryModule is
uint256 _pubKeyX,
uint256 _pubKeyY,
string calldata _keyId
) external override returns (address) {
) external returns (address) {
PassKeyId storage passKeyId = smartAccountPasskey[msg.sender];

if (passKeyId.pubKeyX != 0 && passKeyId.pubKeyY != 0)
Expand Down Expand Up @@ -81,7 +81,7 @@ contract PasskeyRegistryModule is
function isValidSignature(
bytes32 signedDataHash,
bytes memory moduleSignature
) public view virtual override returns (bytes4) {
) public view virtual returns (bytes4) {
return
isValidSignatureForAddress(
signedDataHash,
Expand Down
23 changes: 7 additions & 16 deletions contracts/smart-account/modules/SecurityPolicyManagerPlugin.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ contract SecurityPolicyManagerPlugin is ISecurityPolicyManagerPlugin {
function checkSetupAndEnableModule(
address, //setupContract
bytes calldata //setupData
) external override returns (address) {
) external returns (address) {
// Instruct the SA to install the module and return the address

bool moduleInstallationSuccess;
Expand Down Expand Up @@ -113,9 +113,7 @@ contract SecurityPolicyManagerPlugin is ISecurityPolicyManagerPlugin {
}

/// @inheritdoc ISecurityPolicyManagerPlugin
function checkAndEnableModule(
address _module
) external override returns (address) {
function checkAndEnableModule(address _module) external returns (address) {
// Reject if the module is not a contract
if (!_module.isContract()) {
revert ModuleIsNotAContract(_module);
Expand Down Expand Up @@ -205,9 +203,7 @@ contract SecurityPolicyManagerPlugin is ISecurityPolicyManagerPlugin {
////////////////////////// SECURITY POLICY MANAGEMENT FUNCTIONS //////////////////////////

/// @inheritdoc ISecurityPolicyManagerPlugin
function enableSecurityPolicy(
ISecurityPolicyPlugin _policy
) external override {
function enableSecurityPolicy(ISecurityPolicyPlugin _policy) external {
if (
_policy == ISecurityPolicyPlugin(address(0x0)) ||
_policy == ISecurityPolicyPlugin(SENTINEL_MODULE_ADDRESS)
Expand Down Expand Up @@ -236,7 +232,7 @@ contract SecurityPolicyManagerPlugin is ISecurityPolicyManagerPlugin {
/// @inheritdoc ISecurityPolicyManagerPlugin
function enableSecurityPolicies(
ISecurityPolicyPlugin[] calldata _policies
) external override {
) external {
mapping(address => address)
storage enabledSecurityPolicies = enabledSecurityPoliciesLinkedList[
msg.sender
Expand Down Expand Up @@ -280,7 +276,7 @@ contract SecurityPolicyManagerPlugin is ISecurityPolicyManagerPlugin {
function disableSecurityPolicy(
ISecurityPolicyPlugin _policy,
ISecurityPolicyPlugin _pointer
) external override {
) external {
if (
_policy == ISecurityPolicyPlugin(address(0x0)) ||
_policy == ISecurityPolicyPlugin(SENTINEL_MODULE_ADDRESS)
Expand Down Expand Up @@ -315,7 +311,7 @@ contract SecurityPolicyManagerPlugin is ISecurityPolicyManagerPlugin {
ISecurityPolicyPlugin _start,
ISecurityPolicyPlugin _end,
ISecurityPolicyPlugin _pointer
) external override {
) external {
mapping(address => address)
storage enabledSecurityPolicies = enabledSecurityPoliciesLinkedList[
msg.sender
Expand Down Expand Up @@ -373,12 +369,7 @@ contract SecurityPolicyManagerPlugin is ISecurityPolicyManagerPlugin {
address _sa,
address _start,
uint256 _pageSize
)
external
view
override
returns (ISecurityPolicyPlugin[] memory enabledPolicies)
{
) external view returns (ISecurityPolicyPlugin[] memory enabledPolicies) {
enabledPolicies = new ISecurityPolicyPlugin[](_pageSize);
uint256 actualEnabledPoliciesLength;

Expand Down
10 changes: 5 additions & 5 deletions contracts/smart-account/modules/SessionKeyManagerModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ contract SessionKeyManager is
mapping(address => SessionStorage) internal _userSessions;

/// @inheritdoc ISessionKeyManagerModule
function setMerkleRoot(bytes32 _merkleRoot) external override {
function setMerkleRoot(bytes32 _merkleRoot) external {
_userSessions[msg.sender].merkleRoot = _merkleRoot;
emit MerkleRootUpdated(msg.sender, _merkleRoot);
}
Expand Down Expand Up @@ -87,7 +87,7 @@ contract SessionKeyManager is
/// @inheritdoc ISessionKeyManagerModule
function getSessionKeys(
address smartAccount
) external view override returns (SessionStorage memory) {
) external view returns (SessionStorage memory) {
return _userSessions[smartAccount];
}

Expand All @@ -99,7 +99,7 @@ contract SessionKeyManager is
address sessionValidationModule,
bytes memory sessionKeyData,
bytes32[] memory merkleProof
) public virtual override {
) public virtual {
SessionStorage storage sessionKeyStorage = _getSessionData(
smartAccount
);
Expand All @@ -122,7 +122,7 @@ contract SessionKeyManager is
function isValidSignature(
bytes32 _dataHash,
bytes memory _signature
) public pure override returns (bytes4) {
) public pure returns (bytes4) {
(_dataHash, _signature);
return 0xffffffff; // do not support it here
}
Expand All @@ -131,7 +131,7 @@ contract SessionKeyManager is
function isValidSignatureUnsafe(
bytes32 _dataHash,
bytes memory _signature
) public pure override returns (bytes4) {
) public pure returns (bytes4) {
(_dataHash, _signature);
return 0xffffffff; // do not support it here
}
Expand Down
Loading