rvierdiiev - Attacker can frontrun SubaccountFactory.newSubaccount and steal user funds

[sherlock-admin]

rvierdiiev

high

Attacker can frontrun SubaccountFactory.newSubaccount and steal user funds

Summary

Because SubaccountFactory.newSubaccount creates account using create function, that means that attacker can frontrun the call in order to get control under that account. And if victim has sent any token or made approvals, then attacker can steal them.

Vulnerability Detail

SubaccountFactory.newSubaccount creates new subaccount for the msg.sender. It uses create function to do that. Once user has created subaccount then he can create some token approvals for that subaccount to be able to open positions. It's possible that user will make this 2 tx one by one. 1.He creates subaccount. 2.He approves USDC to the subaccount.

Attacker can watch to such transactions and when he detects them, then he reorders in following way. 1.Attacker creates subaccount(so now he controls that account address as create is used). 2.Victim approves USDC to attackers subaccount.

As result, victim has approved subaccount where owner is attacker. So now attacker can use Subaccount.execute in order to steal those tokens.

Impact

Attacker can steal funds.

Code Snippet

https://github.com/sherlock-audit/2023-04-jojo/blob/main/smart-contract-EVM/contracts/subaccount/SubaccountFactory.sol#L40-L41 https://github.com/sherlock-audit/2023-04-jojo/blob/main/smart-contract-EVM/contracts/subaccount/Subaccount.sol#L45-L58

Tool used

Manual Review

Recommendation

Create subaccount using create2 with salt that contains msg.sender info.

[JoscelynFarr]

fix link: https://github.com/JOJOexchange/smart-contract-EVM/commit/7e2a8fe43ad3280246f71fbc7b4aacd8020276dd

[securitygrid]

Escalate for 10 USDC This is not valid M. The reason is: The user creates a subaccount via newSubaccount, and the address is returned only after the tx is confirmed. How can the user approve token to other's address? The only condition where this can happen is a reorg attack. This is a chain problem.

[sherlock-admin]

Escalate for 10 USDC This is not valid M. The reason is: The user creates a subaccount via newSubaccount, and the address is returned only after the tx is confirmed. How can the user approve token to other's address? The only condition where this can happen is a reorg attack. This is a chain problem.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[arbitrary-cr0]

Escalate for 10 USDC

This issue is invalid because Arbitrum uses a centralized sequencer where transactions are ordered on a FCFS (first come first serve) basis and the mempool that holds all pending transactions is private. Meaning it is currently impossible to frontrun transactions on Arbitrum. Therefore, it is impossible for an attacker to know if there is a pending newSubaccount transaction in the mempool, and even if the attacker somehow did know that, they would be unable to submit a new transaction to frontrun the call because of the FCFS transaction ordering.

[arbitrary-sparrow]

Escalate for 10 USDC

Reposting @arbitrary-cr0 so it gets picked up by the bot:

This issue is invalid because Arbitrum uses a centralized sequencer where transactions are ordered on a FCFS (first come first serve) basis and the mempool that holds all pending transactions is private. Meaning it is currently impossible to frontrun transactions on Arbitrum. Therefore, it is impossible for an attacker to know if there is a pending newSubaccount transaction in the mempool, and even if the attacker somehow did know that, they would be unable to submit a new transaction to frontrun the call because of the FCFS transaction ordering.

[sherlock-admin]

Escalate for 10 USDC

Reposting @arbitrary-cr0 so it gets picked up by the bot:

This issue is invalid because Arbitrum uses a centralized sequencer where transactions are ordered on a FCFS (first come first serve) basis and the mempool that holds all pending transactions is private. Meaning it is currently impossible to frontrun transactions on Arbitrum. Therefore, it is impossible for an attacker to know if there is a pending newSubaccount transaction in the mempool, and even if the attacker somehow did know that, they would be unable to submit a new transaction to frontrun the call because of the FCFS transaction ordering.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Trumpero]

Agree with the escalations. This issue should be considered invalid.

[hrishibhat]

Result: Invalid Unique Based on the comments above on not being able to frontrunning on arbitrum considering this issue invalid

[sherlock-admin]

Escalations have been resolved successfully!

Escalation status:

• securitygrid: accepted
• ArbitraryExecution: accepted

[IAm0x52]

Fix looks good. Creating a sub account now utilizes create2 instead of create so it can no longer be front run