Code Review Beacon

[storming0x]

Scope:
-ApeRegistryBeacon.sol
-ApeBeacon.sol
-Timelock.sol

repo link: https://github.com/coordinape/coordinape-protocol/tree/feat/registry_beacon/contracts/ApeProtocol/wrapper/beacon

[storming0x]

Question:

Does this need to check for address 0x0 or is it meant to allow renouncing ownership too?

coordinape-protocol/contracts/ApeProtocol/wrapper/beacon/ApeBeacon.sol

Line 22 in 3fe0e9d

function transferProxyOwnership(address _newOwner) external {

[storming0x]

Comment:

Care should be taken here on implementations that can be self destructed like in this issue:

https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680

coordinape-protocol/contracts/ApeProtocol/wrapper/beacon/ApeRegistryBeacon.sol

Line 36 in 3fe0e9d

function pushNewImplementation(address _newImplementation) public itself {

I guess here the mitigation is easier since you can't brick the proxy since the beacon stores the implementation pref, and even on a bricked implementation (self destructed) you can then deploy a new implementation and each owner can switch to that one thru this:

coordinape-protocol/contracts/ApeProtocol/wrapper/beacon/ApeBeacon.sol

Line 29 in 3fe0e9d

function setBeaconDeploymentPrefs(uint256 _value) external {

[storming0x]

Overall Implementation looks sound to me, can't see any glaring issue aside from commented above.

The owner of beacon is implied to be trusted to correctly provide safe implementations in this architecture i think is an understood assumption.