search

coordinape / coordinape-protocol

🏆 Coordinape contracts
MIT License
32 stars 12 forks source link

Code Review Beacon #17

Open storming0x opened 2 years ago

storming0x commented 2 years ago

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

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

storming0x commented 2 years ago

Question:

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

https://github.com/coordinape/coordinape-protocol/blob/3fe0e9d96083d97bf92d3939834d2ee90764e43e/contracts/ApeProtocol/wrapper/beacon/ApeBeacon.sol#L22

storming0x commented 2 years ago

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

https://github.com/coordinape/coordinape-protocol/blob/3fe0e9d96083d97bf92d3939834d2ee90764e43e/contracts/ApeProtocol/wrapper/beacon/ApeRegistryBeacon.sol#L36

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:

https://github.com/coordinape/coordinape-protocol/blob/3fe0e9d96083d97bf92d3939834d2ee90764e43e/contracts/ApeProtocol/wrapper/beacon/ApeBeacon.sol#L29

storming0x commented 2 years ago

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.