search

code-423n4 / 2023-03-aragon-findings

0 stars 0 forks source link

`_createProposal` is suspicious of the reorg attack #159

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/ded3784e93e4189cf5bd08e5dd2b82da292b60ba/packages/contracts/src/core/plugin/proposal/Proposal.sol#L33 https://github.com/code-423n4/2023-03-aragon/blob/ded3784e93e4189cf5bd08e5dd2b82da292b60ba/packages/contracts/src/core/plugin/proposal/ProposalUpgradeable.sol#L33 https://github.com/code-423n4/2023-03-aragon/blob/ded3784e93e4189cf5bd08e5dd2b82da292b60ba/packages/contracts/src/core/plugin/proposal/Proposal.sol#L53 https://github.com/code-423n4/2023-03-aragon/blob/ded3784e93e4189cf5bd08e5dd2b82da292b60ba/packages/contracts/src/core/plugin/proposal/ProposalUpgradeable.sol#L53

Vulnerability details

Description

The createProposal functions creates proposals and determined their ids using the _createProposal=>_createProposalId, where the id derivation logic depends only on the proposalCounter value.

At the same time, block reorg may happen on any blockchain. A very clear example to consider would be Polygon:

Please note, reorg on Polygon happens really often. Some are 1 block long, some are >5 minutes long. For the latest, it is quite enough to create the dao and transfer funds to it, especially when someone uses a script, and not doing it by hand.

Example, the Polygon had an incident of 5.5 minutes long reorg 2 weeks ago.

Another example, the incident of 4 minutes long reorg back in December.

While Ethereum seems to be stable, there is no guarantee of the absence of reorgs, which happened a lot previously.

Another interesting impact of this is Optimistic rollups that are quite popular nowadays. Network operators can include a transaction in a block, the user receives the block confirmation and makes a transaction, but the operator reorganizes the blocks for the sake of profit.

All in all, the block reorg is expected behavior for the blockchain and should handle properly to avoid loss of user funds.

Attack scenario

Imagine that Alice creates a proposal, and then votes for it/executes it. Bob sees that the network block reorg happens and calls createProposal. Thus, it creates proposal with an id for which Alice votes. Then Alices' transactions are executed and Alice votes for Bob's proposal. Also, there is possibility to front-run the Alices' transaction using MEV.

Impact

Any significant reorg incident creates an opportunity to steal users' votes (and therefore funds). Also, the use of a small number of confirmations in user transactions can lead to the same problem.

If users rely on the proposal id derivation in advance, any votes/execution of such proposal could potentially be stolen by anyone else. All in all, it could lead to execution of malicious proposals.

Recommended Mitigation Steps

Determine the proposal id as hash of the proposal parameters and proposal creator address.

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Insufficient quality

Barichek commented 1 year ago

Hi! @code423n4 @c4-judge @0xean

I do not agree with the resolution "Insufficient quality".

As mentioned in Severity Categorization section of the code4rena docs, Medium severity stands for 

Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

The report clearly described "hypothetical attack path with stated assumptions, but external requirements". Moreover, the described scenario of consecutive calls of the createProposal and vote functions is absolutely natural usage of the functionality. Especially when a user uses a script to do so. Also, we have provided the real statistics on block reorgs, not even talking about MEV opportunities in different chains.

Please also take a look into the Rabbit Hole M-01 issue that was closed at first, but later rated to medium and rewarded accordingly.

Barichek commented 1 year ago

@0xean

Just pinging you so you don't accidentally miss my message above.

0xean commented 1 year ago

I disagree that your other issue should have been awarded. This is an inherent part of most blockchains and anyone executing critical functions against state, should also be aware of how many blocks have passed and what it infers for the finality (or lack of finality) of that state.

vladbochok commented 1 year ago

@0xean thank you for your answer.

While it is true that users should be aware of the finality of their transactions and the state of the blockchain, this does not absolve the responsibility of the contract to be resilient to known attack vectors. The vulnerability as it stands could impact the function of the protocol or its availability, as well as potentially leak value through the described attack path. This aligns with the criteria for a Medium severity issue, as stated in the code4rena documentation.

I would like to add that the Aragon documentation doesn't mention that the user must wait for the block finality to call the vote function. As well as it is not expected behavior, since usually user may send a couple of transactions on top of the non-finalized state.

In conclusion, we believe that the identified issue is a valid concern and should be reconsidered for its potential impact on the Aragon protocol. A possible mitigation step, as suggested in the original report, is to determine the proposal id using a hash of the proposal parameters and the proposal creator's address, which would increase the resiliency of the implementation against block reorg attacks.

Thank you for your attention, and we look forward to your response.

0xean commented 1 year ago

https://docs.code4rena.com/roles/judges/how-to-judge-a-contest#notes-on-judging

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. In such events, leave a comment on the issue:

As per the c4 docs, will downgrade to QA

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

0xean marked the issue as grade-c

vladbochok commented 1 year ago

@0xean With all due respect, I must disagree.

I understand your point regarding the general nature of inherent vulnerabilities, such as front-running, sandwich attacks, and MEV. However, I would like to clarify why this specific issue goes beyond these general concerns and warrants a Medium severity classification.

While it is true that block reorgs and MEV are inherent aspects of blockchain technology, the vulnerability in question arises due to the specific implementation of the _createProposal function within the Aragon contracts. This implementation choice makes the contracts unnecessarily susceptible to reorg attacks, which goes beyond the general risks faced by all smart contracts.

I would also ask the @c4-sponsor to take a look into this issue to get a better picture :)

0xean commented 1 year ago

My decision here is final unless the sponsor feels differently and I will revisit.

vladbochok commented 1 year ago

@novaknole20 kindly ask to check the report

0xean commented 1 year ago

To clarify, I do think the mitigation is valid and does mitigate what I see as a very small risk that does not meet the C4 criteria for a M severity per the documentation already shared.

vladbochok commented 1 year ago

To clarify, I do think the mitigation is valid and does mitigate what I see as a very small risk that does not meet the C4 criteria for a M severity per the documentation already shared.

Thanks for your time. I did my best to explain why the problem is real and actually leads to a loss of money under external requirements, but is more than QA. Our opinions differ, so the sponsor will help with the resolution.

0xean commented 1 year ago

It is not the sponsors role to settle a disagreement, as the judge, I have the final say here. That being said, if the sponsor disagrees with my assessment, I will reconsider my stance, but may ultimately land at the same decision.

c4-judge commented 1 year ago

0xean marked the issue as grade-b

vladbochok commented 1 year ago

@0xean I appreciate the progress on this issue. Also, I understand that the final decision is on you, but I still can admit its fairness. I will leave my last message here, and will not continue this discussion.

You state that the bug matches the criteria of QA severity.

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.

Front running, MEV, and block reorganization an attack family that touches most of the protocols. Protocols are aware of potential problems and are protected or not protected from such attacks, depending on severity, protocol design, and the nature of the blockchain itself.

  1. Uniswap protects its users from sandwich attacks using price slippage. The absence of such protection leads to real losses of users, therefore, such a bug has a signification impact with corresponding severity.
  2. The ERC20 tokens protect the users from the front-running spent allowance while increasing it.

The examples show the specific attack using the front running, now saying the general blockchain concern that users should be aware of. The same applies to our report. We have shown the specific attack opportunity on one of the most important protocol functions.

I see the difference between general concern and specific attack. While it is required the external requirements, it is not a theoretical condition. It was shown in the original report.

Lastly, the #12 report perfectly emphasized that Aragon Protocol is a DAO framework. It can be used in different conditions by different projects on different networks. Therefore, the concern is stronger here, since we do not see a specific use.

cc @novaknole20

novaknole20 commented 1 year ago

We acknowledge the problem you are describing. However, we agree with @0xean's assessment that

This is an inherent part of most blockchains and anyone executing critical functions against state, should also be aware of how many blocks have passed and what it infers for the finality (or lack of finality) of that state.

As a mitigation, the creator of a proposal can pick a _startDate that is far enough in the future so that the re-org probability effectively vanishes. Therefore, we don't see this as a risk and, most likely, won't change the way how proposalId is computed.

vladbochok commented 1 year ago

@novaknole20

This is an inherent part of most blockchains

1) It is inherited part same as reentrancy attack, frontrunning and etc. Where is the difference? 2) The nature of the bug doesn't really fix the impact though :) And I didn't see any comments about the impact, only about " inherent part"

As a mitigation, the creator of a proposal can pick a _startDate that is far enough in the future so that the re-org probability effectively vanishes. Therefore, we don't see this as a risk and, most likely, won't change the way how proposalId is computed.

I'm most concerned that you are not even going to fix the problem. Users at least should be aware that they need to wait for finality to vote (quite hard to imagine with the reorg tendency of Polygon and the average finality of L2s nowadays).

Barichek commented 1 year ago

Thank you very much for your attention to this report and for your time.

Taking into account

I don't understand your feeling that this issue is a QA, let alone your reluctance to fix it to eliminate completely unnecessary attack vectors (that are more than real).