createNewShare and asDFactory#create are vulnerable to chain reorganizations

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L114-L118 https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/asD/src/asDFactory.sol#L34

Vulnerability details

Impact

After calling createNewShare and asDFactory#create, users will usually deposit funds into the contracts. In events of chain reorganization, users who createdNewShare / createdAsD and shortly bought a share / minted asD, may lose some of their funds.

This is possible particulatly due to asDFactory#create using CREATE, which does not depend on the inputs nor the caller; similarly, createNewShare depends only on how many shares have been previously created.

createNewShare

Share creation is restricted at deployment, but, per README, it could be unrestricted as well. This example assumes that it is unrestricted.

1. Alice createsNewShare with id == x

2. Bob notices the ongoing reorg (or just does it by coincidence), and createsNewShare with the same id = x at the same time

3. Alice tries to buy one share.

4. Reorg is resolved. The chain where Bob created share with id == x becomes canonical. Alice's createNewShare txn is discarded.

5. Alice buy txn is mined. She ends up buying Bob's share. Even if she sells it, she would still lose funds because of buying/selling fees.

In the worst case, Bob could set such bonding curve that would make Alice spend her whole allowance for Market contract. Considering she pays 10% fee two times (buying and selling), this would make her lose 1/5 of whatever her initial allowance was before the reorg.

asDFactory#create

1. Alice creates asD "ALICE" on address 0xabcd, derived from asFactory's address and its nonce.

2. At the same time, Bob creates asD "BOB" on the same address 0xabcd.

3. Alice shortly deposits her NOTE into what she thinks is her own contract, to mint asD "ALICE".

4. Reorg is resolved in Bob's favor.

5. Few weeks later, Alice finds out that her NOTE has been generating yield for Bob and not for herself.

Recommended Mitigation Steps

For asDFactory#create, use create2 with salt derived from msg.sender and user-provided salt.

For createNewShare, derive id from the hash of parameters and msg.sender.

Assessed type

Timing

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

OpenCoreCH marked the issue as disagree with severity

[c4-sponsor]

OpenCoreCH (sponsor) acknowledged

[OpenCoreCH]

These are extremely improbable, constructed examples. You could probably create such an example for most protocols, especially the one with the consecutive IDs because many smart contracts use them. If a user wants to be really sure that they are on the canonical chain and this does not happen, they can simply wait a few blocks.

[MarioPoneder]

According to our guidelines:

Unless there is something uniquely novel created by combining vectors, most submissions regarding vulnerabilities that are inherent to a particular system or the Ethereum network as a whole should be considered QA. Examples of such vulnerabilities include front running, sandwich attacks, and MEV.

Also reorg-attacks are part of this category, therefore QA seems most appropriate.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-c

[osmanozdemir1]

Hi @MarioPoneder Thanks for judging this contest. I am commenting here under the primary issue as the author of one of the duplicates (#312) of this finding.

I acknowledge these kind of issues are related to blockchain itself, and a person could create such an example for most protocols as the sponsor mentioned. However, the upscaling factor related to this protocol is that contract creations are totally permissionless here, which makes this protocol much more vulnerable to this issue.

Based on that sentence in the guideline, frontrunning or MEV issues too would be invalid. However these issues still valid because protocols should take precautions to protect their users. Similar to that, if a protocol allows any user to create contracts permissionlessly, that protocol should also implement protections against the creation mechanism.

The problem here is not reorg itself, it is the protocol not taking necessary precautions while allowing everyone to deploy contracts.

I also would like to point out similar submissions related to permissionless contract creations and reorgs are historically judged as medium. References: August 2023 - PoolTogether: https://github.com/code-423n4/2023-08-pooltogether-findings/issues/31 July 2023 - PoolTogether: https://github.com/code-423n4/2023-07-pooltogether-findings/issues/416 May 2023 - Maia: https://github.com/code-423n4/2023-05-maia-findings/issues/861 April 2023 - Frankencoin: https://github.com/code-423n4/2023-04-frankencoin-findings/issues/155 January 2023 - RabbitHole: https://github.com/code-423n4/2023-01-rabbithole-findings/issues/661

I would appreciate if you could reconsider the severity of this issue and its duplicates. Kind regards

[MarioPoneder]

Hi and thank you for your comment and the referenced findings!

After not finding a suitable answer in the org repo, I did some additional research on the C4 Judges Discord channel.
The answer is it depends on how exposed a protocol is to such an attack and how likely/expensive and long (in terms of blocks) such a reorg could be. Therefore I kindly ask for additional sponsor input on this. @OpenCoreCH

According to the README:

The code will be deployed to Canto

Looking at Cantoscan there is a Last finalized block and a Last safe block.
According to What are Ethereum commitment levels? Latest, Safe, Finalized:

The safe block is a block that has received attestations from two-thirds of Ethereum’s validator set. Safe blocks are understood as unlikely to be reorged. For example, one of the few ways safe blocks could experience a chain reorganization is if there is a large-scale, coordinated attack on the network.

I see it as the user's / front-end's responsibility to wait for the Last safe block. Although the protocol does not take precautions for a reorg attack, it's not clear if this is necessary on the Canto chain like on Polygon for example where reorgs are regular.

Therefore, I will finalize my decision after the sponsor has provided additional input about their definition of Last safe block, the occurrence of reorgs on Canto and for how many blocks they last.

[hungdoo]

Hi @MarioPoneder , thanks for the judging.

My report at the #405 duplicate can contribute to the ongoing discussion in several ways:

1. The sponsor has confirmed that while asDFactory is currently deployed only on CANTO, the 1155tech market is open for deployment on other chains where reorgs occur frequently. The Last safe block to consider could be in minutes. For instance, the Polygon network has recorded several reorgs a day involving hundreds of transactions.

2. We should avoid placing the responsibility on users to wait for the safest time to opt-in to the market after createNewShare was executed. This is because the 1155tech market operates on a BondingCurve pricing mechanism which increases token price over the number of tokens minted. It's in the user's best interest to act in a First-Come-First-Serve (FCFS) manner. If users wait for the safe block, they risk missing out on a good price as others decide to opt in as soon as possible (via other means, i.e., interacting directly with the smart contract). This could discourage them from engaging with the market.

[OpenCoreCH]

After not finding a suitable answer in the org repo, I did some additional research on the C4 Judges Discord channel. The answer is it depends on how exposed a protocol is to such an attack and how likely/expensive and long (in terms of blocks) such a reorg could be. Therefore I kindly ask for additional sponsor input on this. @OpenCoreCH

Good question, had to look it up, but it looks like these reorgs are not possible. Canto is a fork of Evmos/Ethermint and also uses Tendermint Core BFT consensus. This provides 1-block finality and not probabilisitic finality like other chains: https://docs.ethermint.zone/core/pending_state.html

[MarioPoneder]

Thanks everyone for their input on this!

Considering the 1-block finality on Canto, QA will be maintained.
However, those finds are still a valuable contribution, therefore moving forward with grade-b.

1155tech may be deployed on other chains in the future.

According to the README (source of truth of this contest), the protocol will be deployed on Canto and I cannot take potential future deployments into consideration for judgement, especially when we don't know exactly which chain it's going to be.

Further info regarding 1-block finality:
One can observe on Cantoscan that Last finalized block and Last safe block are always the same.

[c4-judge]

MarioPoneder marked the issue as grade-b