delvtech / council Goto Github PK

The only way to be removed from the GSC today is for your voting power to drop below the required threshold, then get kicked. If voting power comes from delegation, you have no control over this.

Ideally, inactive voters that don't want to be on the GSC will naturally drop below the threshold and be kicked, but adding the ability to manually leave is trivial and gives the voter more control.

How important is it that DAY_IN_BLOCKS matches actual block time? Keep in mind that block times will be 12 seconds post merge:

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/CoreVoting.sol#L15

https://github.com/element-fi/elf-council/blob/3ca2a3242dd6dee2bd6facfc072bb503d92f3800/contracts/CoreVoting.sol#L20

This mapping returns 0 for default, we should make it private and then add a modified version of the default getter which returns the default mapping instead of zero.

Credit: @daejunpark

History.sol

• Params in Natspec comments aren’t in the same order as the code:
  
https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/libraries/History.sol#L333

simpleProxy.sol

• Needs natspec comment for _newGovernance param

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/simpleProxy.sol#L41

OptimisticGrants.sol

• @param _amount The amount to add to the solvency. Shouldn’t it be the amount to remove from the solvency?

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/features/OptimisticGrants.sol#L45

OptimisticRewards.sol

• Do we need 3 slashes for the unused param?

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/vaults/OptimisticRewards.sol#L76

VestingVault.sol

• Missing NatSpec comment for _who param:

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/vaults/VestingVault.sol#L312

• Some internal methods have NatSpec comments and some don’t

Missing:_getWithdrawableAmount()

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/vaults/VestingVault.sol#L383

Incomplete: _syncVotingPower()

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/vaults/VestingVault.sol#L317

https://github.com/element-fi/elf-council/blob/3ca2a3242dd6dee2bd6facfc072bb503d92f3800/contracts/CoreVoting.sol#L226

The current event will emit a true for events which are deleted but are positive without meeting quorum, these events don't actually get executed making this event hard to parse. We should add a second bool field for quorum passing.

Credit: @daejunpark

ICoreVoting.sol

Missing natspec comments
https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/interfaces/ICoreVoting.sol#L5

ILockingVault.sol

Missing natspec comments
https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/interfaces/ILockingVault.sol#L5

IVotingVault.sol

Has natspec comments

The README included a broken link to our old Immunefi bounty program. Once we've landed on a new vendor, we should add updated details to the README.

related to: delvtech/hyperdrive#975

The method is called largeSpend, but the contract variables are highSpendLimit:

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/features/Spender.sol#L74

The comment calls it large spend limit even though the param is highSpendLimit:

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/features/Spender.sol#L32

File: contracts\features\OptimisticGrants.sol
83: /// WARNING: A contract _recipient will log msg.sender as this contract's address // REVIEW: Note, outdated comment? Which _recipient?

File: contracts\libraries\History.sol
079: require(data < uint256(1) << 192, "OoB"); // REVIEW: Note, require statement could be made more readable with data <= type(uint192).max.

File: contracts\libraries\VestingVaultStorage.sol
03: import "./Storage.sol"; // REVIEW: Note, import is not used.

File: contracts\vaults\GSCVault.sol
23: uint256 public idleDuration = 60 * 60 * 24 * 4; // REVIEW: Note, instead of '60 * 60 * 24 * 4' use '4 days' for readability.

94: members[msg.sender].joined // REVIEW: Optimization, no need to rewrite joined, just update votingVaults.

File: contracts\vaults\LockingVault.sol
141: emit VoteChange(fundedAccount, delegate, int256(amount)); // REVIEW: Minor, wrong order of params in VoteChange.

163: emit VoteChange(msg.sender, delegate, -1 * int256(amount)); // REVIEW: Minor, wrong order of params in VoteChange.

186: emit VoteChange(msg.sender, oldDelegate, -1 * int256(userBalance)); // REVIEW: Minor, wrong order of params in VoteChange.

192: emit VoteChange(msg.sender, newDelegate, int256(userBalance)); // REVIEW: Minor, wrong order of params in VoteChange.

160: // Add the newly deposited votes to the delegate // Note: the comments are not relevant (copy of comments from deposit function).

162: // Emit a event to track added votes // Note: the comments are not relevant (copy of comments from deposit function).

185: // Emit a event to track added votes // Note: the comments are not relevant (copy of comments from deposit function).

File: contracts\vaults\OptimisticRewards.sol
27: uint256 public challengePeriod = 60 * 60 * 24 * 7; // REVIEW: Note, instead of '60 * 60 * 24 * 7' user '7 days' for readability.

File: contracts\vaults\VestingVault.sol
145: address _delegatee = _delegatee == address(0) ? _who : _delegatee; // REVIEW: Note, no need to redeclare _delegatee.

CoreVoting.sol

• Comity spelled wrong

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/CoreVoting.sol#L105

Timelock.sol

• Comity should be spelled committee

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/features/Timelock.sol#L22

History.sol

• blocknumber user (Not sure what this means)

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/libraries/History.sol#L147

• spelling: minium -> minimum

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/libraries/History.sol#L280

OptimisticRewards.sol

• Should be “would take effect” (not affect)

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/vaults/OptimisticRewards.sol#L53

VestingVault.sol

• Spelling: olyManager modifier -> onlyManager modifier

https://github.com/element-fi/council/blob/d371ac82184fc26e19e958bd0b861f83e74b2955/contracts/vaults/VestingVault.sol#L74

This would make it possible to see what actions have been registered.

It would also make it possible to see what actions are being proposed in CoreVoting. When the actions are hashed off chain, we can only see a callHash for Timelock proposals.

A batch of calls may be doubly executed in Timelock.

Scenario

Consider the following two scenarios where increaseTime() is executed after execute() for the same callHash:

1. The authorized GSC contract accidentally or maliciously calls increaseTime() for the callHash that has already been executed. (Difficulty: High)
2. OR, the GSC legitimately calls increaseTime() near the end of the original waiting period, but increaseTime() is delayed for some reason until the original waiting period passes, and immediately adversaries front-run execute() before the increaseTime() transaction. (Difficulty: Medium)

Then, callTimestamps[callHash] becomes non-zero again, and the subsequent execute() may lead to double execution of the same batch of calls.

Recommendation

Add an extra condition require(callTimestamps[callHash] != 0); in the increaseTime() function to ensure that the given callHash is active.

Currently proving membership puts the entire voting power on a time delay. This makes topping-up votes to maintain membership difficult.

simple solution:
Implement a top-up method that does not adhere to the locking period.

https://github.com/element-fi/elf-council/blob/3ca2a3242dd6dee2bd6facfc072bb503d92f3800/contracts/CoreVoting.sol#L199

Currently votes last forever till a contract call deletes them, plus they can be deleted after the min voting time has passed. This means (1) we need constant execution on chain to remove old proposals (2) even though we allow longer voting times it's hard to coordinate.

We should swap so that the proposal is unlocked after a min voting time and expired after max voting time. It can be executed during the period between min and max but only deleted without execution after the max.

In the current design, merely having more YES votes than that of NO votes is enough to execute a proposal. This means that a proposal will be executed, even if we have only a single YES ballot, zero NO ballot, and a lot of MAYBE ballots. There may exist scenarios where this behavior is problematic, and it may be considered to require a certain minimum quorum (ratio) of YES ballots.