Wallet deployment is vulnerable to cross-chain frontrunning

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L57
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L34
https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccountFactory.sol#L38

Vulnerability details

Impact

Wallet deployment is vulnerable to cross-chain frontrun and front-run

Proof of Concept

We need to look into the wallet deployment function.

function deployCounterFactualWallet(address _owner, address _entryPoint, address _handler, uint _index) public returns(address proxy){
	bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));
	bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
	// solhint-disable-next-line no-inline-assembly
	assembly {
		proxy := create2(0x0, add(0x20, deploymentData), mload(deploymentData), salt)
	}
	require(address(proxy) != address(0), "Create2 call failed");
	// EOA + Version tracking
	emit SmartAccountCreated(proxy,_defaultImpl,_owner, VERSION, _index);
	BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
	isAccountExist[proxy] = true;
}

/**
 * @notice Deploys wallet using create and points it to _defaultImpl
 * @param _owner EOA signatory of the wallet
 * @param _entryPoint AA 4337 entry point address
 * @param _handler fallback handler address
*/ 
function deployWallet(address _owner, address _entryPoint, address _handler) public returns(address proxy){ 
	bytes memory deploymentData = abi.encodePacked(type(Proxy).creationCode, uint(uint160(_defaultImpl)));
	// solhint-disable-next-line no-inline-assembly
	assembly {
		proxy := create(0x0, add(0x20, deploymentData), mload(deploymentData))
	}
	BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);
	isAccountExist[proxy] = true;
}

the deploy wallet use create opcode to create smart contract, the deployCountFactualWallet uses create2 to create smart contract.

An important difference lies in how the address of the new contract is determined. With CREATE the address is determined by the factory contract's nonce. Everytime CREATE is called in the factory, its nonce is increased by 1. With CREATE2, the address is determined by an arbitrary salt value and the init_code.

The wallet deployment is vulnerable to cross-chain front-run.

For example, a user use deployWallet to deploy a wallet in chain A,

The user assume he also controls the wallet in chain B so he sent the asset to chain B's wallet address that has not been deployed yet.

A hacker detects his transaction and frunt-run the wallet deployment in chain B.

A hacker can know the nonce of factory contract by seeing how many transaction is deployed via the factory contract.

The hacker can then copy the deploymentData and front-run the user in chain B and become owner of the wallet.

Precisely what happen in the wintermute hack back last year.

https://rekt.news/wintermute-rekt/

For the wallet that use create2, a malicious user can extract salt because he know the salt is computed below

bytes32 salt = keccak256(abi.encodePacked(_owner, address(uint160(_index))));

and he can copy the deploymentData and deploy a wallet in chain B and call BaseSmartAccount(proxy).init to become the owner of the smart account.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol add chain id when generating salt and not use create opcode when deployment the proxy and when the wallet address is pre-computed in getAddressForCounterfactualWallet in Factory contract.

bytes32 salt = keccak256(abi.encodePacked(_owner, block.chainid, address(uint160(_index))));

[c4-judge]

gzeon-c4 marked the issue as duplicate of #460

[livingrockrises]

"A hacker can know the nonce of factory contract by seeing how many transaction is deployed via the factory contract.

The hacker can then copy the deploymentData and front-run the user in chain B and become owner of the wallet."

I don't think they can become the owner of the wallet in chain B (there is a way by modified entry point/ handler then take over but not the way you described here)

[livingrockrises]

lack of proof

[c4-sponsor]

livingrockrises marked the issue as sponsor disputed

[c4-judge]

gzeon-c4 changed the severity to 3 (High Risk)

[c4-judge]

gzeon-c4 marked the issue as satisfactory