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

5 stars 4 forks source link

Operator role can update a policy without going through the governance / kernel update execution mechanism #490

Closed code423n4 closed 1 year ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L586

Vulnerability details

Impact

A privileged role can make contract updates that should go through governance

Proof of Concept

In OlympusDao updates to policies and modules are made by the kernel and are only callable by the governanceExecutors that only executes updates voted by the governance. The documentation specifies that all contract updates go through the kernel.

" In this framework, all contract dependencies and authorizations are managed via “Actions” in the Kernel.sol contract. " (from contest docs)

In the operator.sol contract there is a function to change the bondCallback policy to another contract. This can be done without notifying the kernel and the bondCallback could be changed to a malicious contract without passing by governance and with the kernel still displaying the old contract. This only happens in this case and is not possible to do for all other policies.

https://github.com/code-423n4/2022-08-olympus/blob/b5e139d732eb4c07102f149fb9426d356af617aa/src/policies/Operator.sol#L586

Recommended Mitigation Steps

Use the kernel set bondCallback policy in operator.sol

Oighty commented 2 years ago

In the current version of the Kernel architecture, Policies do not have the known of other Policy dependencies. There are a few places where a Policy interacts with another Policy in the system (Heart -> Operator, Operator -> BondCallback). While the BondCallback contract can be updated on the Operator contract via the permissioned role, the permissioned role cannot install the new Policy in the Kernel. The expected order of ops here is to have Governance install the new BondCallback Policy and then have the permissioned role update the Operator reference to it. This may be changed in a future version.

0xean commented 1 year ago

closing as invalid / as designed.