code-423n4 / 2023-01-biconomy-findings

12 stars 10 forks source link

Deny of service in `SmartAccountFactory` #464

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

The salt used for create2 does not include information from the init method, so it is vulnerable to front-running.

Proof of Concept

it's impossible to override an existing contract in Ethereum. From EIP-684:

If a contract creation is attempted, due to either a creation transaction or the CREATE (or future CREATE2) opcode, and the destination address already has either nonzero nonce, or nonempty code, then the creation throws immediately, with exactly the same behavior as would arise if the first byte in the init code were an invalid opcode. This applies retroactively starting from genesis.

For that reason if the salt is repeated, the call fill fault.

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

If the user want to deploy a wallet using deployCounterFactualWallet with the _owner=A and _index=0, an attacker can listen the memory pool and deploy with an higer gas, a wallet with the same owner and index, but with different _entryPoint and _handler so the attacker can deny the original call, and make unwanted actions in the proxy.

BaseSmartAccount(proxy).init(_owner, _entryPoint, _handler);

Recommended Mitigation Steps

Include all received fields in the salt calculation.

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #460

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor confirmed

c4-judge commented 1 year ago

gzeon-c4 marked the issue as satisfactory

c4-judge commented 1 year ago

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