High 1: ack, we disagree with severity of this issue given intended design as explained. since expectation is that setup is immutable w create2 on both timelock and serpentor, if in the future a migration is needed from serpentor it would need a new contract that can add the 'AcceptThrone' option as noted. A migration to a new serpentorBravo contract would be the way to mitigate this, but serpentor currently doesn't have a way to migrate to a new timelock as per design.
Informational 9: ack. Although serpentor and time lock are design to work together, it could be the case that the eta is calculated from a different on-chain gov contract other than serpentor and thus the assert may still be needed to ensure delay is enforced.
Informational 14: ack. Tried refactoring line into an internal method but gas cost increased too much for our taste. We think it’s ok compromised to keep these methods in a single line duplicated for gas efficiency.
Informational 15: ack. This is meant to follow closely reference implementation , in case of migrating is meant to keep the time lock and migrate to a new version of serpentor
Informational 17: ack. This is meant to follow reference implementation not assuming time lock and serpentorBravo will always work together or can work with other similar versions.
Informational 22: ack. This is a valid point that we will consider in future versions of serpentor, for now the mitigation for token changing supply is to redeploy serpentor
Change Log:
High 1: ack, we disagree with severity of this issue given intended design as explained. since expectation is that setup is immutable w create2 on both timelock and serpentor, if in the future a migration is needed from serpentor it would need a new contract that can add the 'AcceptThrone' option as noted. A migration to a new serpentorBravo contract would be the way to mitigate this, but serpentor currently doesn't have a way to migrate to a new timelock as per design.
High: 2 b611dc056b408484f8f2137a0aae7d2cf03a87e4
Medium 1: Pending Question
Informational 1: b611dc056b408484f8f2137a0aae7d2cf03a87e4
Informational 2: b611dc056b408484f8f2137a0aae7d2cf03a87e4
informational 3: 5c7a3d260043db42baec4ae97db39bf1386919f5
Informational 4 1b865fdbd20a76227ec0fb17b002da26a8f01e46
Informational 5: ack, is meant to be compatible with GovernorBravo/alpha interface and constant CAPS is more readable internally
Informational 6: 2287b4299aac7907f27ecf68706ceb67e5e94aa8 CompoundBravo has similar issue? https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorBravoDelegate.sol#L224
Informational 7: 8f378c042af1ac13a8c964e50eabf9a83555742a
Informational 8: 5a440463a0773c31a801d647b8c558a6cf4637ad
Informational 9: ack. Although serpentor and time lock are design to work together, it could be the case that the eta is calculated from a different on-chain gov contract other than serpentor and thus the assert may still be needed to ensure delay is enforced.
Informational 10: ack. This is meant to follow the reference implementation from compound where the same logic is used for blocks and timestamp https://github.com/compound-finance/compound-protocol/blob/master/contracts/Timelock.sol https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorBravoDelegate.sol#L216
Informational 11: ack. Added documentation to knight role to make clear is whitelist guardian 7da0f56f8df57e8c0f3200f2229d148b69fd11b5
Informational 12: ack. This is meant to follow closely reference implementation here: https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorBravoDelegate.sol#L237
Informational 13: d1c32b6b0292b490e6e675ef0d82f5ba3e3fe9fe
Informational 14: ack. Tried refactoring line into an internal method but gas cost increased too much for our taste. We think it’s ok compromised to keep these methods in a single line duplicated for gas efficiency.
Informational 15: ack. This is meant to follow closely reference implementation , in case of migrating is meant to keep the time lock and migrate to a new version of serpentor
Informational 16: this is meant to follow the reference implementation which also reverts https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Timelock.sol#L101
Informational 17: ack. This is meant to follow reference implementation not assuming time lock and serpentorBravo will always work together or can work with other similar versions.
Informational 18: ack. this is meant to follow reference implementation and compatibility at event indexing level https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorBravoInterfaces.sol#L7
Informational 19: 2fc5f7e8873d4ee19fdd2bfcd0082e3c42cf4d68
Informational 20: ack. This is meant to follow the reference implementation which avoids this check for voting mitigations and gas efficiency https://github.com/compound-finance/compound-protocol/blob/master/contracts/Governance/GovernorBravoDelegate.sol
Informational 21: ack. Yes, this is also the case in reference implementation AFAIK https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorBravoDelegate.sol#L157
Informational 22: ack. This is a valid point that we will consider in future versions of serpentor, for now the mitigation for token changing supply is to redeploy serpentor
Informational 23: ad56db48eb361bf31507859b8132f89d5a6f98c2
Informational 24: this follows the reference implementation also which I guess is mitigated during voting process to avoid duplicated transactions with small changes https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorBravoDelegate.sol#L134
Informational 25: ack, this follows the reference implementation logic for security reasons very closely, if there’s a way to optimize without impacting the logic too much, we are keen to explore the option. https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Governance/GovernorBravoDelegate.sol#L270
Informational 26: ack, although we agree with recommendation we wanted to keep indexing compatibility with reference compound time lock https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Timelock.sol#L12
Informational 27: eb84a3c34749364f45b97da00167cb28b4c195dd
Informational 28: ack, this follows the reference implementation here https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Timelock.sol#L93