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

Multi owned ecdsa remediations #189

Merged
merged 3 commits into from
Jan 16, 2024
Merged

Multi owned ecdsa remediations #189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5972152
Info fixes
Jan 15, 2024
dfbce44
remove comment
Jan 15, 2024
c5d20eb
name fix
Jan 16, 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
12 changes: 6 additions & 6 deletions contracts/smart-account/modules/MultiOwnedECDSAModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ contract MultiOwnedECDSAModule is
{
using ECDSA for bytes32;

string public constant NAME = "ECDSA Ownership Registry Module";
string public constant NAME = "Multiowned ECDSA Ownership Module";
string public constant VERSION = "0.2.0";

// owner => smartAccount => isOwner
Expand All @@ -45,7 +45,7 @@ contract MultiOwnedECDSAModule is
}
uint256 ownersToAdd = eoaOwners.length;
if (ownersToAdd == 0) revert NoOwnersToAdd();
for (uint256 i; i < ownersToAdd; ) {
for (uint256 i; i < ownersToAdd; ++i) {
if (eoaOwners[i] == address(0))
revert ZeroAddressNotAllowedAsOwner();
if (_smartAccountOwners[eoaOwners[i]][msg.sender])
Expand All @@ -55,9 +55,7 @@ contract MultiOwnedECDSAModule is
);

_smartAccountOwners[eoaOwners[i]][msg.sender] = true;
unchecked {
++i;
}
emit OwnershipTransferred(msg.sender, address(0), eoaOwners[i]);
}
numberOfOwners[msg.sender] = ownersToAdd;
return address(this);
Expand All @@ -70,9 +68,9 @@ contract MultiOwnedECDSAModule is
) external override {
if (_isSmartContract(newOwner)) revert NotEOA(newOwner);
if (newOwner == address(0)) revert ZeroAddressNotAllowedAsOwner();
if (owner == address(0)) revert ZeroAddressNotAllowedAsOwner();
if (owner == newOwner)
revert OwnerAlreadyUsedForSmartAccount(newOwner, msg.sender);
//address(0) is not an owner initially as it points to the address(0) = false
if (!_smartAccountOwners[owner][msg.sender])
revert NotAnOwner(owner, msg.sender);
if (_smartAccountOwners[newOwner][msg.sender])
Expand All @@ -91,6 +89,7 @@ contract MultiOwnedECDSAModule is
unchecked {
++numberOfOwners[msg.sender];
}
emit OwnershipTransferred(msg.sender, address(0), owner);
}

/// @inheritdoc IMultiOwnedECDSAModule
Expand All @@ -101,6 +100,7 @@ contract MultiOwnedECDSAModule is
unchecked {
--numberOfOwners[msg.sender];
}
emit OwnershipTransferred(msg.sender, owner, address(0));
}

/// @inheritdoc IMultiOwnedECDSAModule
Expand Down
6 changes: 1 addition & 5 deletions contracts/smart-account/modules/PasskeyRegistryModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,7 @@ contract PasskeyRegistryModule is
if (passKeyId.pubKeyX != 0 && passKeyId.pubKeyY != 0)
revert AlreadyInitedForSmartAccount(msg.sender);

smartAccountPasskey[msg.sender] = PassKeyId(
_pubKeyX,
_pubKeyY,
_keyId
);
smartAccountPasskey[msg.sender] = PassKeyId(_pubKeyX, _pubKeyY, _keyId);

return address(this);
}
Expand Down
2 changes: 1 addition & 1 deletion lib/forge-std
Open in desktop
Submodule forge-std updated 16 files
+1 −0 .gitattributes
+1 −1 package.json
+666 −0 scripts/vm.py
+11 −3 src/StdChains.sol
+2 −2 src/StdCheats.sol
+4 −0 src/StdJson.sol
+61 −33 src/StdUtils.sol
+927 −415 src/Vm.sol
+216 −0 src/mocks/MockERC20.sol
+221 −0 src/mocks/MockERC721.sol
+2 −6 test/StdChains.t.sol
+1 −1 test/StdCheats.t.sol
+3 −1 test/StdError.t.sol
+15 −0 test/Vm.t.sol
+441 −0 test/mocks/MockERC20.t.sol
+721 −0 test/mocks/MockERC721.t.sol
2 changes: 1 addition & 1 deletion lib/registry
Open in desktop
Submodule registry updated 3 files
+116 −40 .gas-snapshot
+6 −6 src/base/Module.sol
+3 −1 src/interface/IModule.sol
27 changes: 27 additions & 0 deletions test/bundler-integration/module/MultiOwnedECDSA.Module.specs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ describe("MultiOwned ECDSA Module (with Bundler):", async () => {
] = [] as SignerWithAddress[];
const smartAccountDeploymentIndex = 0;
const SIG_VALIDATION_SUCCESS = 0;
const ADDRESS_ZERO = "0x0000000000000000000000000000000000000000";
let environment: BundlerTestEnvironment;

before(async function () {
Expand Down Expand Up @@ -172,6 +173,32 @@ describe("MultiOwned ECDSA Module (with Bundler):", async () => {
await multiOwnedECDSAModule.isOwner(userSA.address, eve.address)
).to.equal(true);
});

it("TransferOwnership from address(0) is not possible", async () => {
const { multiOwnedECDSAModule, entryPoint, userSA } = await setupTests();

// Calldata to set 0x00..00 as owner
const txnData1 = multiOwnedECDSAModule.interface.encodeFunctionData(
"transferOwnership",
[ADDRESS_ZERO, eve.address]
);
const userOp = await makeEcdsaModuleUserOp(
"execute_ncC",
[multiOwnedECDSAModule.address, 0, txnData1],
userSA.address,
smartAccountOwner1, // can be signed by any owner
entryPoint,
multiOwnedECDSAModule.address,
{
preVerificationGas: 50000,
}
);

await environment.sendUserOperation(userOp, entryPoint.address);
expect(
await multiOwnedECDSAModule.isOwner(userSA.address, eve.address)
).to.equal(false);
});
});

describe("removeOwner():", async () => {
Expand Down