Biconomy - Smart Contract Wallet contest - gogo's results

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)
+   }

Lines of code

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

Vulnerability details

Uninialized or front-runnable .init function in proxy implementation contract

Impact

DoS for all users' smart account proxies leading to locked funds forever.

Proof of Concept

Nowhere in the code the SmartAccount.sol implementation contract is initialized by calling the .init function. This means that the owner of the SmartAccount.sol implementation contract is unknown (as it is set in the .init function) or the .init function may not even be called - can be forgotten by the team and later called or front-runned by an attacker. This will give an external account control over the implementation control such as executing any transaction from it via call and delegatecall.

If that happens, the attacker can immediately sign and execute a transaction directly on the SmartAccount.sol implementation contract (via call) using .execTransaction and make a delegatecall by setting the tx.operation to 1 and tx.to to a contract containing a selfdestruct operation. This will block all proxies and stuck all users's funds forever due to DoS and non-upgradeability as the updateImplementation mechanism is in the destroyed SmartAccount.sol implementation contract.

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)
+   }

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");

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");

Lines of code

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

Vulnerability details

Impact

Results in unexpected behavior in the EntryPoint contract.

Proof of Concept

As said in the official specification of EIP-4337: "If the account does not support signature aggregation, it MUST validate the signature is a valid signature of the userOpHash, and SHOULD return SIG_VALIDATION_FAILED (and not revert) on signature mismatch. Any other error should revert.". SmartAccount._validateSignature does the opposite and reverts if the signature is invalid which will be wrong interpret by the EntryPoint contract.

Tools Used

Manual review

Recommended Mitigation Steps

Return SIG_VALIDATION_FAILED instead of revert in SmartAccount._validateSignature:

    function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash, address)
    internal override virtual returns (uint256 deadline) {
        bytes32 hash = userOpHash.toEthSignedMessageHash();
        //ignore signature mismatch of from==ZERO_ADDRESS (for eth_callUserOp validation purposes)
        // solhint-disable-next-line avoid-tx-origin
-       require(owner == hash.recover(userOp.signature) || tx.origin == address(0), "account: wrong signature");
+       if(owner != hash.recover(userOp.signature) && tx.origin != address(0)) return SIG_VALIDATION_FAILED;
        return 0;
    }

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/main/scw-contracts/contracts/smart-contract-wallet/paymasters/verifying/singleton/VerifyingSingletonPaymaster.sol#L77-L90

Vulnerability details

Cross-Chain Signature Replay Attack

Impact

User operations can be replayed on smart accounts accross different chains. This can lead to user's loosing funds or any unexpected behaviour that transaction replay attacks usually lead to.

Proof of Concept

As specified by the EIP4337 standard to prevent replay attacks ... the signature should depend on chainid. In VerifyingSingletonPaymaster.sol#getHash the chainId is missing which means that the same UserOperation can be replayed on a different chain for the same smart contract account if the verifyingSigner is the same (and most likely this will be the case).

Tools Used

Manual review

Recommended Mitigation Steps

Add the chainId in the calculation of the UserOperation hash in VerifyingSingletonPaymaster.sol#getHash

    function getHash(UserOperation calldata userOp)
    public view returns (bytes32) { // @audit change to view
        //can't use userOp.hash(), since it contains also the paymasterAndData itself.
        return keccak256(abi.encode(
                userOp.getSender(),
                userOp.nonce,
                keccak256(userOp.initCode),
                keccak256(userOp.callData),
                userOp.callGasLimit,
                userOp.verificationGasLimit,
                userOp.preVerificationGas,
                userOp.maxFeePerGas,
                userOp.maxPriorityFeePerGas
		block.chainid // @audit add chain id
            ));
    }