code423n4 commented 1 year ago

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 uses setOwner function as one-step ownership change. This has high risks. The code of setOwner function is in the below:

FIle:  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");
111         address oldOwner = owner;
112         owner = _newOwner;
113         emit EOAChanged(address(this), oldOwner, _newOwner);
114     }

setOwner function only requires _newOwner != address(0). However other addresses can make things wrong. For example, if a non-existent EOA account address is set as the new owner, the SmartAccount wallet will lose control. All assets in the wallet are stuck in it forever. Nobody can take it.

Other wrong addresses can be incorrect/non-existent smart contract addresses. If the new owner is set to one of these wrong addresses, the consequence is the same as the above one.

Since the expected users of SmartContract wallets are normal users, it is highly probable that they supply a wrong address to setOwner when changing ownership. It is just like things happening on the blockchain - lots of tokens/ethers locked up on the chain due to supplying wrong addresses.

The another thing is that if a non-existent address is set as the owner of a SmartAccount wallet, all assets will be lost, not just the assets carried in one transaction. So this is quite serious.

If a non-existent address is set to the owner of a SmartAccount wallet, there is no way to recover/rescue it. The privileged functions with onlyOwner modifier are not callable since there is no owner available to sign the transaction. Functions protected by mixedAuth are not callable either since there is no precoded function available to call. This means the issue cannot be addressed by updating contract as these functions are protected by mixedAuth.

SmartAccount.execTransaction() can execute delegate calls from SmartAccount, which can possibly reset the ownership by setting the value of the owner state variable. However, since the transaction has to be signed by the SmartAccount owner which is no longer available, SmartAccount.execTransaction() cannot solve the above problem.

Proof of Concept

The owner of a SmartAccount wallet calls setOwner(_newOwner) function and _newOwner is a non-existent address. The ownership will be successfully changed to _newOwner. From this point on, the SmartAccount wallet lost the control. None of its privileged functions can be successfully called. The SmartAccount contract cannot be upgraded since upgrading requires privileged function call. All assets are frozen in the wallet forever.

Tools Used

Manual audit.

Recommended Mitigation Steps

Take a two-step ownership change procedure: (1) the current owner stores the intended new owner address in a state variable; (2) the new owner calls the setting function to set it as the new owner. The setting function must check that the msg.sender is the new owner.

c4-judge commented 1 year ago

gzeon-c4 changed the severity to QA (Quality Assurance)