SmartAccount implementation contract can be destroyed by owner

[code423n4]

Lines of code

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

Vulnerability details

SmartAccount implementation contract can be destroyed by owner

Impact

Locking users' funds forever due to DoS for all deployed smart account proxies. Neither implementation upgrade will be possible nor withdrawing funds.

Proof of Concept

The expected behaviour of interacting with the SmartAccount.sol contract is that all of its function will be called by proxies via delegatecall meaning that the execution context will always be the proxy contracts. But there is no modifier nor check (require/if) that functions are not called directly on the SmartAccount implementation contract via call such as this one.

This opens the opportunity for the implementation contract owner to call .execTransaction directly on the SmartAccount contract via call and pass a transaction that delegatecalls to a malicious contract that implements a selfdestruct operation.

The SmartAccount implementation contract address is passed to the SmartAccountFactory through the constructor which means that the implementation contract is deployed before that and the owner is unknown.

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

[GeorgeHNTR]

@gzeon-c4 I'm not sure if this is an exact duplicate of 496 as this one talks about a centralisation issue - the owner of the SmartAccount implementation contract can selfdestruct the contract by following the steps above. Issue 496 and its duplicates talk about an attacker being able to exploit this by calling the .init function to steal ownership if contract is not initialized and then execute the selfdestruct exploit. The key point is that their recommendations are that protocol devs call .init in the implementation contract and set the owner themselves. This will not solve the above problem because the owner can still perform the exploit even though it is not a random malicious user, it can still be a malicious protocol account or the private key can be compromised. That's why I reported it as a separate issue for an owner who destroys the implementation contract, not for someone who steals the ownership and then destroys the contract. So even if .init is called on the implementation contract, and even if the owner is set by the protocol, whoever controls the owner address can still destroy the implementation contract and cause DoS. In my opinion, there should be an explicit validation in the contract that the owner cannot call functions on the implementation contract. This can be achieved by adding a modifier like this one or as stated in the above problem - by setting the owner of the implementation contract to the address of the factory in the factory itself, because the factory cannot perform any transactions to the implementation contract once it is deployed.

[gzeon-c4]

I am aware of you have submitted #491 and decided to consider this as a duplicate. The owner of the implementation contract should be renounced instead of having a centralized owner.