Uninialized or front-runnable `.init` function in proxy implementation contract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L166 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L17

Vulnerability details

Uninialized or front-runnable .init function in proxy implementation contract

Impact

DoS for all users' smart account proxies leading to locked funds forever.

Proof of Concept

Nowhere in the code the SmartAccount.sol implementation contract is initialized by calling the .init function. This means that the owner of the SmartAccount.sol implementation contract is unknown (as it is set in the .init function) or the .init function may not even be called - can be forgotten by the team and later called or front-runned by an attacker. This will give an external account control over the implementation control such as executing any transaction from it via call and delegatecall.

If that happens, the attacker can immediately sign and execute a transaction directly on the SmartAccount.sol implementation contract (via call) using .execTransaction and make a delegatecall by setting the tx.operation to 1 and tx.to to a contract containing a selfdestruct operation. This will block all proxies and stuck all users's funds forever due to DoS and non-upgradeability as the updateImplementation mechanism is in the destroyed SmartAccount.sol implementation contract.

Tools Used

Manual review

Recommended Mitigation Steps

The standard way a factory pattern works is that the factory deploys the implementation contract it its constructor and then calls init to become the owner of this implementation contract. As the factory does not implement a function such as f.e. executeArbitraryTransaction there is no way the .execTransaction is called directly on the SmartAccount by the owner. Even more that way there can not be a valid signature to pass the checkSignatures validation.

Create the implementation contract in the SmartAccountFactory.constructor instead of externally passing it. Then call the .init function on the newly deployed implementation contract and set the owner to be the SmartAccountFactory address (address(this))

-   constructor(address _baseImpl) {
-       require(_baseImpl != address(0), "base wallet address can not be zero");
-       _defaultImpl = _baseImpl;
-   }

+   constructor(address _entryPoint, address _handler) {
+       _defaultImpl = address(new SmartAccount());
+       SmartAccount(_defaultImpl).init(address(this), _entryPoint, _handler)
+   }

[c4-judge]

gzeon-c4 marked the issue as duplicate of #496

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory