code-423n4 / 2022-08-olympus-findings

5 stars 4 forks source link

Governance functions relies on the proposal Id which says nothing about the proposal content #474

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L180 https://github.com/code-423n4/2022-08-olympus/blob/main/src/policies/Governance.sol#L205

Vulnerability details

Impact

There are submitProposal/endorseProposal/activateProposal functions on the Governance smart contract. The functions accept the proposed, which does not contain any information about the proposal. As a result, transactions of users can be front-ran.

Proof of Concept

Imagine someone sending two transactions to the mempool: submitProposal and endorseProposal with expected by them proposalId (see note section below). Then the malicious user can send a transaction submitProposal with different proposal content and a higher fee. This transaction most likely will be included in the block before the transaction from a non-malicious user.

So, it is possible that the order of transaction will be:

Note, that it is not necessarily for the user to send two transactions to mempool simultaneously to get under attack. That also can happen while chain reorg.

Recommended Mitigation Steps

It is better to use a hash of proposal content as a unique identifier. Then it would be impossible to manipulate or front-run transactions to the governance.

fullyallocated commented 2 years ago

the proposal ID is generated in the function and stored when it's called, so the second proposal will just have a different proposal ID.

0xean commented 1 year ago

closing as invalid.