code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/OwnerProxy.sol#L9-L36

Vulnerability details

Impact

OwnerProxy's selfdestruct

Proof of Concept

While only the owner (the timelock) can call the execute function, this doesn't mean it can't be compromised or phished to call a malicious _target, which could contain a call to selfdestruct().

As selfdestruct() would be a simple OPCODE in the context of the OwnerProxy contract (which is the one using delegatecall() in execute()), this would destroy the contract.

This is a known bug in the community (see the Parity Multisig Hack): delegatecalls from contracts are dangerous.

Recommended Mitigation Steps

Consider making OwnerProxy a library instead of a contract to protect it from being selfdestructed and to further protect its state (that can also be manipulated as a contract)

Alternatively, consider deploying the OwnerProxy contract using CREATE2 so that the contract could be re-created at the same pre-computed address, if need be

obatirou commented 2 years ago

Disputed and disagree with severity

Nothing different from transferring the ownership to another address in terms of severity It's about wardens appreciation of our ownership architecture versus ours. We can imagine many other malicious scenarios, assuming that the Multisig/Timelock/OwnerProxy combination is not enough to prevent the protocol from being compromised.

jack-the-pug commented 2 years ago

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/README.md#L109

In order to complete the ownership architecture, we need the OwnerProxy contract in charge of executing scripts for the Timelock (run transactions atomically).

Per the doc, OwnerProxy is a utils for gov, FWICS, arb code exe is a feature, not a bug.

Will downgrade to QA.

code-423n4 / 2022-06-nested-findings