scopelift / flexible-voting Goto Github PK

Since OpenZeppelin has released version 5.0.0 of their contracts, which includes changes to the Governor contracts, we should update Flexible Voting as-needed for compatibility with this release and tag a release of our own.

Of particular note to pay attention to is the addition of a nonce for voting with params, which came in direct response to our reporting of the replay vulnerability found in FV by Trail of Bits during the Frax governance audit. Since OZ has added this, it may be the case we can remove it from our FV implementation altogether.

Details here

It's currently ~8kB over the limit¹. As part of this, let's also bring back the size check in CI which as removed in #46 (comment)

To fix we probably want to:

• extend Comet to redefine updateBasePrincipal, note that this function will need to call internal functions that we do not have the space to include in Comet -- namely FlexVotingClient._checkpointRawBalanceOf! Need to figure out how to do this (e.g. maybe make the function external but with a modifier that only allows the Comet instance to call it?)
• create a new Comet constructor that stores the governor address
• extend Comet to add the storage vars in FlexVotingClient. This might be a little tricky because some of them reference types in libraries like OZ's Checkpoints and we cannot afford to import them:

  mapping(uint256 => mapping(address => bool)) private proposalVotersHasVoted;
  
  /// @notice Map proposalId to vote totals expressed on this proposal.
  mapping(uint256 => ProposalVote) public proposalVotes;
  
  /// @notice Mapping from address to stored (not rebased) balance checkpoint history.
  mapping(address => Checkpoints.History) private balanceCheckpoints;
  
  /// @notice History of total stored (not rebased) balances.
  Checkpoints.History internal totalDepositCheckpoints;

• extend CometExt with the remaining logic we need

Specific checklists we should go through:

• https://github.com/nascentxyz/simple-security-toolkit/blob/main/audit-readiness-checklist.md
• https://secure-contracts.com/
• https://github.com/transmissions11/solcurity

Details here: #11 (comment)

• #47

• port over remaining tests from AToken

• Test that people who supply collateral cannot express votes

• What happens if someone tries to call expressVote when they have borrowed the base asset? We need to make sure that they cannot express a preference. From here:

  Account balances for the base token are signed integers. An account balance greater than zero indicates the base asset is supplied and A BALANCE LESS THAN ZERO INDICATES THE BASE ASSET IS BORROWED (emphasis mine)

  This should be fine because the FlexVotingClient.expressVote function checks that getPastStoredBalance > 0 and reverts otherwise. But we should have a test to confirm.

• If we want to be really thorough we should test that each of the cToken supply/withdraw functions modifies the internal cToken checkpointing properly

The usecase is:

• Alice holds GOV, a governance token
• Alice has delegated her GOV to Bob to vote on her behalf
• Alice hears that she can earn yield on her GOV while still maintaining her voting rights on Aave, b/c they have ATokenFlexVoting
• Alice deposits GOV into Aave and receives aGOV
• Alice still wants Bob to be able to vote on her behalf
• So Alice delegates to Bob through aGOV
• Now when a new proposal is created, Bob can express a voting preference to the atoken with Alice's weight added to his

Thanks to 0xmashroom for the good idea!

See https://docs.soliditylang.org/en/latest/assembly.html#memory-safety for more info

To ensure compatibility, we should use the comment-based /// @solidity memory-safe-assembly approach over the assembly ("memory-safe") approach

Since OpenZeppelin recently released version 5.0.0 of the OZ contract library, and we intend to create a release that is compatible with that one sometime in the near future, we should also create a tag a release that is compatible with the last release of the OZ 4.x.x contracts. This should be done before the new 5.0.0 compatible release of Flexible Voting is done.

As we know all too well, governors change and the governor address associated with a given governance token and/or timelock can be updated over time.

For this reason, the FlexVotingClient abstract contract should reflect this. Specifically:

• the GOVERNOR variable should be mutable, not immutable
• the abstract contract should define a virtual function to update the GOVERNOR value -- we might call it updateGovernor -- which child contracts will be required to implement

This is also needed for better proxy support. There currently is no way to set the GOVERNOR value in an initializer.

This was raised here and here

RE https://github.com/ScopeLift/flexible-voting/pull/21/files#r1052653037

We are re-defining some of the same interfaces in several locations. We should probably clean that up and generally just think about how we want this repo structured/organized

Context: https://github.com/ScopeLift/flexible-voting/pull/11/files#r1020455790

Pull the AToken, Comet, and FractionalPool strategies, along with their test suites, into a new flexible-voting-clients repository. Cleanup code comments (Change from OZ to ScopeLift, add licenses, etc...) and publish a new patch version of the library.

In relevant AToken methods (mint, burn, transferOnLiquidation, others?) read the relevant ReserveData from the Pool contract after the operation completes and store snapshots of it.

When calculating weight, lookup appropriate snapshot data and reimplement the call to getReserveNormalizedIncome (i.e. getNormalizedIncome under the hood, which is internal) to calculate the balance at the snapshot time.

Potentially complicated, requires code duplication, but provides an exact weight.

Most of our Aave-related tests are fork tests. For this reason, we don't fuzz over their inputs -- as this would generate a massive (and expensive) number of RPC calls. But it's possible to fuzz using a deterministic seed, which would allow us to cache RPC calls and avoid unnecessary network interactions.

Two tasks here:

• support deterministic fuzzing in CI
• convert existing Aave-related fork tests into fuzz tests

RE: #21 (comment)

This is intended as a discussion, not something that will definitely be implemented.

Using arbitrary GOV token and Aave receipt token aGOV for the below.

Problem

Currently, aGOV token holders vote until X blocks before the proposal deadline (the CAST_VOTE_WINDOW), to leave time for the rolled up vote to be cast. Block numbers are used because that's how the OZ governor tracks proposals. This has a few issues:

1. It doesn't work as cleanly on L2s or chains like Avalanche where block production is not at a fixed rate. You have fewer guarantees of how much time will remain to cast the rollup vote.
   1. On chains like Optimisim where 1 transaction == 1 block, an attacker can see when CAST_VOTE_WINDOW is, and once it's reached, spam transactions to accelerate the block number and end proposal voting sooner, to prevent the rolled up flexible vote from being cast. (Note that in the case of Optimism, once Bedrock is released, it will be a fixed 2 second block time, but the problem still exists for other chains)
2. Voter UX takes a hit, so flexible voting isn't "free" to implement:
   1. Users have no guarantee someone will actually cast their rollup vote unless they send it themselves, so must pay attention after voting to then submit the rolled up transaction if they want to guarantee their vote is cast.
   2. Users have a smaller voting window (i.e. less time to research a proposal and vote on it) when voting with aGOV compared to GOV voters.
3. Tracking proposal vote counts for an ongoing proposal now lags: Expressed aGOV votes are not cast until near the deadline, so other voters have less information about existing votes, and there may be a large, last minute change once the rollup vote is cast. You can solve this with a UI/data layer that aggregates across all flexible voting contracts, but (1) that's complex, and (2) there's no guarantee those votes will all be cast.

Solution

Modify GovernorCountingFractional so accounts can vote multiple times, but cannot re-cast votes. Assuming I have 10 GOV tokens, here are four scenarios illustrating UX that is possible (ignoring the abstain option for simplicity in the examples):

1. Casting a single yes vote for all 10 tokens
2. Casting an initial vote of 7 yes votes and a second vote of 3 no votes
3. Casting an initial vote of 7 tokens, split into 5 yes and 2 no, a second vote of 3 tokens split into 1 yes and 2 no.
4. Casting an initial vote of 5 tokens, split into 4 yes and 1 no, a second vote of 3 tokens split into 1 yes and 2 no, a third vote of 2 tokens split into 0 yes and 2 no.

As an individual, you likely would never do anything except the first bullet. But this is useful for flexible voting to mitigate all of the above issues.

This solution does not mean you introduce the ability to change votes after they've been cast—once an initial vote with a weight of n tokens is cast, you cannot re-vote with those n tokens. Once my initial vote of 7 tokens has been cast, I can only submit 3 more votes. I cannot replace/update those 7 votes (unless we want to allow that, but feels like a separate issue, and I don't think we want to allow that).

This does mean voting is more expensive and has an cold extra storage write in the governor (to subtract a user's remaining vote weight), but the contract (e.g. AToken) no longer needs to write the vote weight since it can just pass it on to the governor, so you save a warm storage write there. So it will still be more expensive, but I think this is worth it, especially because it mitigates an attack vector. (Granted, isn't that attack vector currently present in a different form? I don't think I've heard this discussed much, but either way I don't think we should introduce it in a second format)

From an implementation perspective I don't think it should be too complicated:

• Pools such as FractionalPool no longer need CAST_VOTE_WINDOW
• GovernorCountingFractional now tracks how many tokens of a users voting weight have been used.

There is very likely other implementation details, and perhaps downsides, that I'm missing that need to be discussed

At the time of making a snapshot (i.e. a balance changing due to a transfer, mint, etc...), record the balanceOf before (pre balance) the relevant event takes place and after (post balance).

When calculating vote weight, look at the closest "post balance" entry recorded before the proposal time, and the closest "pre balance" entry recorded after the proposal time, and linearly infer a balance between them. If there is no entry recorded after the proposal was submitted (because the user has not take any actions since the proposal was created), look at the balanceOf value at the time of the weight being looked up, and use this as the second point in the linear inference.

Pros:

• Maybe simpler than #8
• Maybe cheaper (gas) than #8

Cons:

• Not exact: total weight could be > or < 100%, and this must be accounted for

• A pool contract allows users to deposit GOV tokens
• GOV tokens can also leave the Pool without being withdrawn by depositors (example: Uni in Compound is leant out to a borrower)
• The contract delegates deposited tokens to itself
• The depositor has the right to vote proportionally based on the tokens they've supplied to the pool
• Depositors can withdraw from the Pool contract
• The Pool contract voting deadline for any proposal must be some time before the deadline of the proposal itself
• After the Pool voting deadline passes, but before the proposal deadline, anyone should be able to trigger the contract to vote based on the For/Against votes that depositors have expressed
• The pool contract should prevent new entrants from being able to vote on proposals created before their entry, or more broadly, a depositor's voting weight in the Pool should reflect their ownership at the time of a proposals creation.
• Depositors voting weight is determined by their proportion of total outstanding deposits, regardless of the current contract's actual balance

The FlexVotingClient is indirectly tested through its usage in the Aave and Comp clients, but those have been moved into a separate repo, and they also have their own specific considerations. We should have a set of unit tests for the base client that live in this repository.

https://github.com/ScopeLift/flexible-voting/pull/27/files#r1063822660

When snapshotting, ignore interest earned (your weight is underweight). We could provide a method for users to force a snapshot whenever they wanted.

We want to implement Flexible Voting for ATokens inside the Aave system. To do so, we'll need a test harness that:

• Forks mainnet to interact with the Aave protocol
• Extends/implements an AToken for a mock ERC20Votes GOV token
• Adds the new GOV AToken market to Aave
• Can supply, borrow, liquidate, etc... for said market

Within aToken implementation test suite setUp functions we should:

• deploy GOV to aave
• deploy our new aToken implementation
• upgrade the aGOV token to our new implementation via upgradeToAndCall
• self-delegate during upgradeToAndCall
• confirm this doesn't change any storage slots

More details in this thread: https://github.com/ScopeLift/flexible-voting/pull/11/files#r1019640978

This would be primarily for those using FV with the hardhat developer framework. We'll want to include any of the latest best practices, such as typescript interface generation, etc...

The FlexVotingClient contract should use signatures that align with the Governor itself, i.e. treating the client almost like a mini-Governor. This is advantageous for a couple of reasons. For one, it is simply easier to reason about if you're already familiar with the Governor contracts and the way they work. Additionally, it allows existing tooling to be leveraged for clients with little-to-no changes, perhaps as far as allowing clients to be imported directly into tools like Tally so users could cast votes there. This effort would closely resemble the work done for L2 Flexible Voting to make the aggregator compatible with the Governor.

We already did this here: aave/aave-v3-core#726

Other functions we should update:

• aToken.burn to make it public (rather than external)
• aToken.intialize to make it virtual (so that we can override flex voting aTokens to self-delegate during initialization)
• aToken._transfer to make it virtual (so that we can checkpoint during both transfer and transferFrom with the same code)

How should functions be arranged within the flex voting atoken? Should we group overridden aToken functions together? Should functions just be grouped logically (e.g. initializer near constructor)?

RE: #21 (comment)

Since introducing partial voting in #31 we need to figure out how to handle the hasVoted function on our Governor extension. It could be confusing to people -- leading them to think that an account had cast all of its votes and was unable to cast additional votes.

More context/discussion (including feedback from Tally) here.

Goals for this ticket:

• look through sourcegraph to see how existing Governor integrations are/aren't using the hasVoted function
• spec out new approach

Set up invariant testing and add invariant tests where appropriate.

For example, testing that the total weight being voted with never exceeds the voting weight delegated by the gov token (would revert if so).

Our current COUNTING_MODE designation is:

Per OZ's IGovernor:

 * There are 2 standard keys: `support` and `quorum`.
     *
     * - `support=bravo` refers to the vote options 0 = Against, 1 = For, 2 = Abstain, as in `GovernorBravo`.
     * - `quorum=bravo` means that only For votes are counted towards quorum.
     * - `quorum=for,abstain` means that both For and Abstain votes are counted towards quorum.

So, our COUNTING_MODE is telling people that only FOR votes will be counted towards quorum. Accordingly, that's exactly what we do:

But do we want this behavior? I don't remember ever making a principled decision about it. Were we just following the bravo default? Is this an artifact of a previous implementation (e.g. in which abstain votes were just inferred from the params passed in)? Either way, this should at least be documented.

There is no nonce by default for castVoteWithReasonAndParamsBySig and _castVote depends on voting only once. This is fine for the normal flow, but partial votes by sig can be replayed in GovernorCountingFractional as long as they are still under the total voting weight of that user. Adding a nonce to bytes memory params and enforcing it would fix this issue.

ERC-165 defines "the interface identifier as the XOR of all function selectors in the interface"

GovernorCountingFractional v1.1.0 added the fractionalVoteNonce public state variable (getter method) which therefore changes it's interface identifier. However, this is not currently reflected in it's supportsInterface method, which just uses the OZ Governor definition

Suggested here https://github.com/ScopeLift/flexible-voting/pull/21/files#r1052654091

RE #11 (comment)