Description

In the EcdsaOwnershipRegistryModule smart contract, we noted that the transferOwnership() function checks to ensure the new owner is an EOA.

However, the same check does not exist in the initForSmartAccount() function, which sets the initial owner.

Impact

This would allow a user to accidentally initialize the EcdsaOwnershipRegistryModule smart contract with a non-EOA owner. This would effectively make the module unusable, as a smart contract cannot produce valid signatures that will pass the isValidSignature() function checks.

Recommendations

We were initially going to recommend adding in the isContract() checks, but we learned that the extcodesize opcode is banned in user operations that have nonempty initCode. Since this would be the case for a user operation that is calling initForSmartAccount(), this is not a valid recommendation.

Instead, we recommend that Biconomy extensively document that the module owner cannot be an EOA. The function parameter is already named eoaOwner, but we would also recommend adding a comment above the function as well as adding it to any documentation related to this module.

Remediation

The Biconomy team have acknowledged this finding, stating that users will be expected to use the Biconomy SDK, which will perform off-chain isContract() checks. They have also stated that they will document this behavior extensively to make it extremely unlikely for users to make this mistake.