Attacker can take control over each SmartAccount proxy and steal all users' funds

[code423n4]

Lines of code

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

Vulnerability details

Attacker can take control over each SmartAccount proxy and steal all users' funds

Impact

All users' funds can be stolen by a single attacker (tx gas cost only)

Proof of Concept

There are 2 main reasons for this vulnerability:

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 proxy, 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 user's smart account proxy. 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 each user's smart account proxy thorugh the fallback function via delegatecall and pass a transaction that either calls setOwner or simply transfer funds to a wallet of the attacker. Because of the missing validation attacker can pass any mock contract address in the expected position in the signature (the r). The attacker 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 user's smart contract proxy, the execution will continue without reverting.

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
2. Call .execTransaction on any user's smart account proxy with tx.to = attacker contract address, tx.operation = 0 and tx.value = proxy contract's balance
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. All user's funds will be transfered to the attacker contract

Note: attacker can iterate though all user's wallet and perform this attack on each Note: attacker can take also take ownership, withdraw ERC20 tokens and basically do everything on user's behalf

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