bcnmy/scw-contracts

SCW contracts for Biconomy Smart Account

scw-contracts's Issues

Bug Report 2024-01-28

I'm opening this issue to report a bug, which I'm doing based on this:

If you've identified a bug that hasn't been reported, open a new Issue detailing the bug.

With this specific bug, it'd be better if the details aren't available for everyone to see, so can I send my writeup privately to the team somehow? I can do email or any other medium.

Wrong name

scw-contracts/contracts/smart-account/modules/MultiOwnedECDSAModule.sol

Line 32 in bb275bf

string public constant NAME = "ECDSA Ownership Registry Module";

Name should be replaced

Wrong information in natspec

For MultiOwnedECDSA module in develop branch (links below) some of the information is used from ECDSA module but they seem inaccurate.

https://github.com/bcnmy/scw-contracts/blob/develop/contracts/smart-account/modules/MultiOwnedECDSAModule.sol#L15

https://github.com/bcnmy/scw-contracts/blob/develop/contracts/smart-account/modules/MultiOwnedECDSAModule.sol#L19
(should Not be one owner per smart account)

same in interfaces
https://github.com/bcnmy/scw-contracts/blob/develop/contracts/smart-account/interfaces/modules/IMultiOwnedECDSAModule.sol#L9

Module Manager: execTransactionFromModule does not emit module events

I noticed that in the module manager's execTransactionFromModule function, it directly uses the _execute function from the executor contract. However, this setup doesn't emit any of module events when a transaction succeeds or fails.

I'm wondering why we're not using the _executeFromModule function, like we do in the execBatchTransactionFromModule method, for consistency.

Checking initializer.length

Why does the SmartAccountFactory include a check for initializer.length > 0 in the deployAccount and deployCounterFactualAccount functions? Given that the initializer is not passed directly and is not optional. Under what circumstances would initializer.length be zero?