QuestFactory is suspicious of the reorg attack

[code423n4]

Lines of code

https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L75
https://github.com/rabbitholegg/quest-protocol/blob/8c4c1f71221570b14a0479c216583342bd652d8d/contracts/QuestFactory.sol#L108

Vulnerability details

Description

The createQuest function deploys a quest contract using the create, where the address derivation depends only on the QuestFactory nonce.

At the same time, some of the chains (Polygon, Optimism, Arbitrum) to which the QuestFactory will be deployed are suspicious of the reorg attack.

• https://polygonscan.com/blocks_forked

Here you may be convinced that the Polygon has in practice subject to reorgs. Even more, the reorg on the picture is 1.5 minutes long. So, it is quite enough to create the quest and transfer funds to that address, especially when someone uses a script, and not doing it by hand.

Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation and already created a quest.

Attack scenario

Imagine that Alice deploys a quest, and then sends funds to it. Bob sees that the network block reorg happens and calls createQuest. Thus, it creates quest with an address to which Alice sends funds. Then Alices' transactions are executed and Alice transfers funds to Bob's controlled quest.

Impact

If users rely on the address derivation in advance or try to deploy the wallet with the same address on different EVM chains, any funds sent to the wallet could potentially be withdrawn by anyone else. All in all, it could lead to the theft of user funds.

Recommended Mitigation Steps

Deploy the quest contract via create2 with salt that includes msg.sender and rewardTokenAddress_.

[c4-judge]

kirk-baird marked the issue as unsatisfactory:
Invalid

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[vladbochok]

I do not agree with the resolution.

The project states that the Polygon is one of the networks where it will be deployed. As the attack scenario shows the reorg is not that rare.

Honest user makes two transactions:

• create quest A
• send funds to the quest A

An attacker notices the ongoing reorg and front runs the honest user transaction on creating the quest. So the final order of transaction is:

1. Malicious user creates quest A
2. Honest user creates quest B (earlier expect that deploy quest A)
3. Honest user funds the quest A

[vladbochok]

@c4-judge @kirk-baird

[kirk-baird]

The recommendation would work, it would mean that address is deterministic and irrelevant of any reorgs.
The reason it works is because the to address when doing the token transfer will be irrelevant of the reorg.

An alternative solution could be to have the front end wait for certain number of blocks before between creating a quest and funding the quest.

[vladbochok]

Thank you for your answer, @kirk-baird!

My main message is that this vulnerability is not "Invalid" or "unsatisfactory". This is more than a real bug (especially on the mentioned chains), and its probability greatly increases if the user uses the script for sequential deployment and financing of the quest. That's why I think it's nothing less than a medium-severity vulnerability.

[c4-judge]

kirk-baird marked the issue as grade-b

[c4-judge]

This previously downgraded issue has been upgraded by kirk-baird

[c4-judge]

This previously downgraded issue has been upgraded by kirk-baird

[kirk-baird]

Apologies I thought your first comment was saying the "Recommendations" are incorrect rather than the judging.

The initial mark of invalid was accidental and intended to be removed. The intended behaviour was to downgrade to QA. The reason for downgrading this was I considered it to be a front-end issue which would be protected by waiting for a certain number of block confirmations between each transaction. On reflection this does not seem to be a solid assumption and there is risk here due to the smart contract design.

The Warden has shown a good recommendation to prevent this kind of issue on the smart contract. Thanks for raising this, I'll upgrade it back to Medium.

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-sponsor]

waynehoover marked the issue as sponsor acknowledged