search

code-423n4 / 2022-12-prepo-findings

0 stars 1 forks source link

Governance address should live in Factory, rather than each Market #280

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarket.sol#L47

Vulnerability details

Impact

When a new PrePOMarket is deployed, ownership of the market is transferred to the inputted governance address.

In the event that there is an issue that requires changing the governance address (ie a hack or migration), this will lead to needing to change each market manually.

It can also lead to issues if the governance address is manually entered incorrect.

Proof of Concept

In the constructor of the market, we call transferOwnership(), sending ownership to the inputted governance address. There is lots of room for error here.

If it changes, we'll need to call transferOwnership() from governance to each market. This may not always be possible.

Tools Used

Manual Review

Recommended Mitigation Steps

The governance address should be set and verified once on the Factory contract. Instead of using an onlyOwner modifier, the markets should call to the Factory to check the governance address.

This will allow migrations to happen in just one place, and immediately update all markets, which is more conducive to what is needed in the event of an emergency governance migration.

Picodes commented 1 year ago

Although I do agree that roles in the prePo system should be centralized to minimize the risk of things going wrong and the maintenance overhead, especially as there are may roles in the current system, this falls within best practices and safety checks, so downgrading to QA

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-12-prepo-findings/issues/256