Reorg & Frontrunning Attack Vulnerability in createTerm()

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOnboarding.sol#L153

Vulnerability details

Impact

The createTerm() function in the LendingTermOnboarding.sol contract is vulnerable to a reorg/frontrunning attack. This function deploys a new LendingTerm contract using OZ's Clone.clone(), which uses create() under the hood. Here, the address derivation depends solely on the LendingTermOnboarding nonce.

The worst-case scenario is an attacker exploiting the predictability of contract creation to manipulate votes, leading to a malicious contract being approved instead of the intended one.

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/main/src/governance/LendingTermOnboarding.sol#L153
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/fd81a96f01cc42ef1c9a5399364968d0e07e9e90/contracts/proxy/Clones.sol#L33

This is a Medium severity issues previously reported here an in the referenced reports https://solodit.xyz/issues/m-09-create-methods-are-suspicious-of-the-reorg-attack-code4rena-pooltogether-pooltogether-git

Proof of Concept

The following sequence of events illustrates the vulnerability:

1. Alice, a user with a significant amount of GUILD tokens, submits a series of transactions in which she:
   • Calls createTerm() with profitable parameters
   • Calls proposeOnboard() to propose its onboarding
   • Calls castVote() to cast her vote in support of her proposal
2. Bob observes Alice's transactions and frontruns her by calling createTerm() with malicious parameters before Alice's transactions are executed.
3. Bob's malicious term gets the address Alice expected to create.
4. Since the proposal ID only depends on the term address, Alice's votes are cast in support of Bob's malicious term.

Tools Used

Manual review

Recommended Mitigation Steps

Use Clones.cloneDeterministic() passing a hash of the proposal parameters as the salt.

Assessed type

Other

[0xSorryNotSorry]

1. ETH Main Network will not have re-orgs
2. If a re-org forms on Arbitrum, Alice's transactions will not be executed. Once Alice calls the transaction again, it will revert due to the term is already created.

[c4-pre-sort]

0xSorryNotSorry marked the issue as insufficient quality report

[c4-judge]

Trumpero marked the issue as unsatisfactory:
Invalid

[0xEVom]

Hi @Trumpero

Re-raising this since as you may be aware reorgs do happen on mainnet https://ethereum.org/en/developers/docs/consensus-mechanisms/pos/attack-and-defense#attacks-by-small-stakeholders

On Arbitrum, an L1 reorg would cause an L2 reorg

As far as I can tell, this is a very unlikely but nevertheless legitimate finding, which the mitigation addresses. Here are several instances in the past year in which similar findings were accepted:
2023-01-rabbithole-findings/M-01
2023-04-frankencoin-findings/M-14
2023-08-pooltogether-findings/M-09

[Trumpero]

@0xEVom Agree that this issue is valid and should be fixed, but it only has low severity. If an attacker creates a term maliciously to receive the votes, this proposal still needs to wait for the 7-day voting duration to be executed. During this time, the Guardian role can cancel it directly, or Guild holders can veto it to cancel, so it doesn't cause any impact.
Additionally, this issue has a very unlikely likelihood, and the issue of the chain system should be considered QA as per C4's docs. I mark this as grade-c because it's grade of combination all QA issues from that warden

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-c