SmartAccount implementation contract can be destroyed by anyone

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L192 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L302-L353 https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L342

Vulnerability details

SmartAccount implementation contract can be destroyed by anyone

Impact

Locking all user's funds forever due to DoS for all functions.

Proof of Concept

There are 2 main reasons for this vulnerability:

1. 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.

2. The .checkSignatures in SmartAccount.sol has a missing require(_signer == owner, "INVALID_SIGNATURE"); check in the first if. What the _signer actually does on line 342 is to verify that ITS OWNERS (not the owner of the SmartAccount implementation contract, but the owners of the _signer in the case where the _signer is another smart contract wallet f.e. gnosis safe) have signed/approved the passed data with the contractSignature. But its never checked the _signer is actually the owner of the biconomy smart account implementation contract. Note that the _signer address is also an externally passed variable and has no validation on it so there is basically a call to an arbitrary (untrusted) contract.

This opens the opportunity for an attacker 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. Because of the missing validation attacker can pass any mock contract address in the expected position in the signature (the r). The contract will return EIP1271_MAGIC_VALUE on .isValidSignature and since there is no check that the _signer on line 342 is the owner of the implementation contract, the execution will continue without reverting. The SmartAccount contract that is used by all proxies will be destroyed forever and since it also holds the implementation logic, all funds deposited to proxies will become stuck forever.

To sum up, any attacker can execute the following steps for no cost (except tx gas cost):

1. Create an attacker contract that
   • mocks isValidSignature(bytes,bytes) and returns EIP1271_MAGIC_VALUE
   • has a function (f.e. kill()) with selfdestruct operation
2. Call .execTransaction on the SmartAccount implementation contract with tx.to = attacker contract address and tx.operation = 1
3. Place the attacker contract address at the expected position in signatures (in the R value) and set the V to 0
4. line 342 will check that the attacker contract returns EIP1271_MAGIC_VALUE and will not check whether the _signer (attacker contract) is the owner
5. .execTransaction will continue and call .execute on the Executor (which is inherited Executor -> ModuleManager -> SmartAccount)
6. The SmartAccount implementation contract will be destroyed forever and all funds locked in smart account proxies will be stuck forever as well

As a reference you can see how Gnosis Safe implement the same function. Notice how after all the if blocks there is this require check. In biconomy's SmartAccount.sol this check is not applied when v=0.

Tools Used

Manual review

Recommended Mitigation Steps

Add the following check in SmartAccount.checkSignatures before calling ISignatureValidator(_signer).isValidSignature:

    require(_signer == owner, "INVALID_SIGNATURE");

[c4-judge]

gzeon-c4 marked the issue as duplicate of #175

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[c4-judge]

gzeon-c4 marked the issue as satisfactory