Single step ownership change poses high risks

[code423n4]

Lines of code

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

Vulnerability details

Impact

Contract SmartAccount performs a one-step ownership change by calling setOwner function, which only verifies _newOwner != address(0):

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

109:    function setOwner(address _newOwner) external mixedAuth {
110:        require(_newOwner != address(0), "Smart Account:: new Signatory address cannot be zero");
            address oldOwner = owner;
            owner = _newOwner;
            emit EOAChanged(address(this), oldOwner, _newOwner);
        }

Making such a critical change in a single step is error-prone and can lead to irrevocable mistakes. Since _newOwner could also be mistakenly set to any valid non-existing account addresses or existing incorrect smart contract addresses, any of which will lead to permenant loss of control on the SmartAccount wallet.

Due the following reasons, this issue is considered as medium severity:

1. Considering the intended users of the SmartAccount wallets, when ownership change is requested, the address of the new owner will have to be provided by ORDINARY users, there is a quite high probability of supplying an incorrect address; This is why there are so many tokens/ethers are locked on blockchian.
2. Also considering that SmartAccount wallet will be used as a convenient flexible smart wallet, it is likely that a SmartAccount wallet contains user's significant assets. Loss of control to the SmartAccount will incur permenant significant loss to users. Users may not be able to accept such loss due to ownership change;
3. Any loss due to ownership change is the entire loss of a SmartAccount wallet instead of the loss of a single transation;
4. And the worse, there is no way to rescue it as long as the incorrect owner address is set;
5. This may have negative implication on the reputation of the SmartAccount wallet.

Proof of Concept

1. A valid non-zero non-existing address _newOwner is used with function call SmartAccount.setOwner(_newOwner) initiated by the current owner. The contract owner is handed to _newOwner.
2. All privileged functions protected by onlyOwner or mixedAuth (e.g. setOwner, updateImplementation, updateEntryPoint, transfer, pullTokens, execute, executeBatch) are no longer callable, since the _newOwner does not exist, there is no way to call the above protected functions. Note: since there is no pre-coded code in the contract to call above functions by the contract itself, the functions protected by mixedAuth are also not callable.
3. The upgrading functions updateImplementation, updateEntryPoint are protected by mixedAuth. They are no longer callable, which means the issue cannot be solved through implementation upgrading.
4. Since _newOwner does not exist, it cannot be used to sign transactions. Thus, the SmartAccount cannot sign transactions using _newOwner to undertake privileged tasks such as changing ownership, transferring assets, etc.
5. Transactions executed through execTransaction function must be signed by the owner directly or indirectly (through EIP1271). Thus, they cannot be used to solve the issue in question.
6. Any assets in the contract are no longer transferrable.
7. There is no way to recover it.

Tools Used

Manual analysis.

Recommended Mitigation Steps

It is important to safely change ownership. A two-step ownership change process is recommended:

• Step1: the owner calls a function to set the new owner's address to a state variable, say _newOwner. The function should be protected by onlyOwner;
• Step 2: the new owner calls the switch owner function to actually switch owner address. The function verifies: msg.sender == _newOwner. This process guarantees that the ownership will be correctly handed to an intended one.

[c4-judge]

Duplicate of https://github.com/code-423n4/2023-01-biconomy-findings/issues/365

[c4-sponsor]

livingrockrises marked the issue as sponsor acknowledged