Potential Front-Running grieving attack in CoinbaseSmartWalletFactory Contract

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWalletFactory.sol#L71-L84

Vulnerability details

Impact

Medium, as it results in a temporary DoS for users of the protocol.

The absence of msg.sender or some other unique parameter in the _getSalt function's computation could lead to a issue where two transactions with the same owners and nonce values result in the same deterministic address. Such an attack could result in temporary DoS for users of the protocol, as transactions intended for different addresses could be reverted due to conflicts in the deterministic address generation.

Proof of Concept

The vulnerability is based on the deterministic address generation mechanism used in the createAccount function of the CoinbaseSmartWalletFactory contract. This mechanism relies on the _getSalt function to generate a unique salt for each account creation request. Without including msg.sender or timestamp or a unique identifier in the salt computation, transactions with the same owners and nonce values could result in the same salt, leading to the same deterministic address. This scenario could be exploited by a front-runner to observe a transaction and submit a similar transaction with a higher gas price, aiming to have it executed first. Here is the relevant code snippet from CoinbaseSmartWalletFactory.sol: https://github.com/code-423n4/2024-03-coinbase/blob/e0573369b865d47fed778de00a7b6df65ab1744e/src/SmartWallet/CoinbaseSmartWalletFactory.sol#L71-L84

function _getSalt(bytes[] calldata owners, uint256 nonce) internal pure returns (bytes32 salt) {
    salt = keccak256(abi.encode(owners, nonce));
}

Tools Used

Manual Review

Recommended Mitigation Steps

I would have suggested that the _getSalt function include msg.sender in its computation as shown below:

```solidity
function _getSalt(bytes[] calldata owners, uint256 nonce) internal pure returns (bytes32 salt) {
    salt = keccak256(abi.encode(owners, nonce, msg.sender));
}
```

But including msg.sender in the salt computation does not inherently resolve the issue because it does not differentiate between transactions initiated by different users. This is because msg.sender is the address of the account that is currently executing the contract, which is the same for all transactions initiated by the same user. Therefore, if two transactions from the same user are made with the same owners and nonce values, they would still result in the same salt, potentially leading to the same deterministic address being targeted by both transactions.

A more robust approach would be to introduce additional unique parameters into the salt computation, such as a timestamp or a unique identifier generated for each transaction. This will ensure that the deterministic address generation mechanism is truly unique for each transaction, regardless of the owners and nonce values. By adding a timestamp or a unique transaction identifier, you introduce a variable that changes with each transaction, thus making it impossible for two transactions to generate the same deterministic address even if they have the same owners and nonce values.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #72

[raymondfam]

See #72.

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid