code-423n4 / 2022-10-inverse-findings

0 stars 0 forks source link

## `gov` role can affect the whole functioning of the `Market` #603

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/Market.sol#L77

Vulnerability details

gov role can affect the whole functioning of the Market

Impact

There are many functions that can only be triggered by onlyGov role, this means losing it will be harmful for the functioning of the system.

So basically in Market.sol we find functions affecting the integrity of the maths in the protocol and therefore the balance of it as:

And the roles/control flow that after all can trigger another methods

As this affects lender and pauseGuardian, next functions would get also affected:

And finally as oracle can be assigned by governance, if governance is wrongly set to a wrong address that gets to know it, this address can change oracle to a malicious one and therefore exploit it getting value from the Market.

gov role is assigned twice in the code. First one in the constructor, second one in setGov(). Both pieces of code doesn't check for 0 address, nor emit events, neither use a 2 steps transfer protocol. This is dangerous as if wrongly assigned the address for any kind of error, governance and all the functions involved

Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyGov() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately.

As all of this scenario is more like hypothetical but can affect funds if the gov is assigned to a malicious address I suggest Medium Risk.

Github Permalinks

https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/Market.sol#L77 https://github.com/code-423n4/2022-10-inverse/blob/cc281e5800d5860c816138980f08b84225e430fe/src/Market.sol#L117-L197

Recommended steps

Recommend use functions for a two-step address change as done in operator:

  1. Approve a new address as a pendingGov
  2. A transaction from the pendingGov address claims the pending ownership change.

This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.

Also, consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.

neumoxx commented 1 year ago

Should be probably QA.

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Overinflated severity