llamaxyz / llama-periphery

🦙 Peripheral smart contracts for operating Llama instances 🦙

Llama Periphery

Llama is an onchain governance and access control framework for smart contracts. This repository contains supporting modules for operating Llama instances. For the core contracts, see the Llama repository.

Modules

Llama modules are extensions to Llama instances that can be adopted by using a Llama action to configure and deploy.

• Token Voting: smart contract policies that allow voting token holders to create actions enforced by delegated token thresholds or collectively approve or disapprove an action through token voting.

Prerequisites

Foundry must be installed. You can find installation instructions in the Foundry docs.

We use just to save and run a few larger, more complex commands. You can find installation instructions in the just docs. All commands can be listed by running just -l from the repo root, or by viewing the justfile.

VS Code

You can get Solidity support for Visual Studio Code by installing the Hardhat Solidity extension.

Installation

$ git clone https://github.com/llamaxyz/llama-periphery.git
$ cd llama
$ forge install

Setup

Copy .env.example and rename it to .env. The comments in that file explain what each variable is for and when they're needed:

• The MAINNET_RPC_URL variable is the only one that is required for running tests.
• You may also want a mainnet ETHERSCAN_API_KEY for better traces when running fork tests.
• The rest are only needed for deployment verification with forge scripts. An anvil default private key is provided in the .env.example file to facilitate testing.

Commands

• forge build - build the project
• forge test - run tests

Deploy and Verify

• just deploy - deploy and verify payload on mainnet
• Run just -l or see the justfile for other commands such as dry runs.

Deployments

Name	Ethereum	Optimism	Arbitrum	Base	Polygon
Factory
LlamaTokenVotingFactory	0xFBE17545dffD75A92A5A72926AE581478973FE65	0xFBE17545dffD75A92A5A72926AE581478973FE65	0xFBE17545dffD75A92A5A72926AE581478973FE65	0xFBE17545dffD75A92A5A72926AE581478973FE65	0xFBE17545dffD75A92A5A72926AE581478973FE65
Governor
LlamaTokenGovernor (logic contract)	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A
Token Adapters
LlamaTokenAdapterVotesTimestamp (logic contract)	0x088C268cb00226D6A9b29e5488905Aa94D2f0239	0x088C268cb00226D6A9b29e5488905Aa94D2f0239	0x088C268cb00226D6A9b29e5488905Aa94D2f0239	0x088C268cb00226D6A9b29e5488905Aa94D2f0239	0x088C268cb00226D6A9b29e5488905Aa94D2f0239

Testnet deployments

Name	Sepolia	Holesky	Base Sepolia
Factory
LlamaTokenVotingFactory	0xFBE17545dffD75A92A5A72926AE581478973FE65	0xFBE17545dffD75A92A5A72926AE581478973FE65	0xFBE17545dffD75A92A5A72926AE581478973FE65
Governor
LlamaTokenGovernor (logic contract)	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A	0x3f3DAB3ab8cEc2FBd06767c2A5F66Cb6BFF21A4A
Token Adapters
LlamaTokenAdapterVotesTimestamp (logic contract)	0x088C268cb00226D6A9b29e5488905Aa94D2f0239	0x088C268cb00226D6A9b29e5488905Aa94D2f0239	0x088C268cb00226D6A9b29e5488905Aa94D2f0239

Smart contract reference

Run the following command to generate smart contract reference documentation from our NatSpec comments and serve those static files locally:

$ forge doc -o reference/ -b -s

Security

Audit

We received an audit from Spearbit. You can find the link to the report below:

• Llama Token Governor Spearbit Audit

Bug bounty program

This repository is subject to the Llama bug bounty program.

Slither

Use our bash script to prevent slither from analyzing the test and script directories.

$ chmod +x slither.sh
$ ./slither.sh

llama-periphery's Issues

Support should be a `uint8` parameter that is compared against a `VoteType` enum

TokenVoting Script

Script to issue policy to TokenVoting Modules.

refactor: cast(Dis)approval and submit(Dis)approval

There seems to be a lot of common code between the castapproval and castdisapproval. And also between submitApproval and submitDisapproval. We should try to pull out common code as much as possible similar to how we did it in the Llama Framework. It'll make it easier to read and understand.

`LlamaTokenVotingFactory` events feedback

Some feedback on the events here so that it's much easier for offchain indexing:

1. Since we've decided below to both deploy the ActionCreator and Caster contracts together, We can combine all 4 events into 1 IMO.
2. We need some additional fields and indexing as below such as the deployer (which will be msg.sender)

event LlamaTokenVotingModuleCreated(address indexed deployer, bool indexed isERC20, address indexed token, address llamaTokenholderActionCreator, address llamaTokenholderCaster, uint256 chainId).

This is also consistent with how we have it in the LlamaFactory

Additionally, the creationThreshold for ActionCreator, and minApprovalPct and minDisapprovalPct for Caster should be emitted in the constructor/initialization function of the ActionCreator and Caster respectively.

Evaluate if token voting architecture support can include dual token voting

Action thresholds and voting/vetoing quorums should be able to be updated

I would lean towards keeping things consistent with other major frameworks here.

docs: add natspec to `getQuorumCheckpoints`

Add natspec to getQuorumCheckpoints

https://github.com/spearbit-audits/review-llama-token-governor/pull/1#discussion_r1432600611

Support default tokens

If your governance token implements ERC20Votes with IERC6372 timestamp based checkpointing then you should be able to specify your token adapter as the zero address. Rather than call the adapter directly, the creator and caster contracts will use internal functions. These internal functions will check if the adapter address is the zero address if it is then it will call the token directly or return default values

Check function compatibility with existing governance tokens.

1. For example ENS doesn't have clock
2. UNI doesn't seem to use any sort of Votes implementation from an initial skim

We should explore a lot of the popular tokens and see what they're missing such that we can support them in a backwards compatible manner.

Revisit the strategy being used for TokenholderCaster in the tests

It should be one with force roles if I remember right. But that's not the case currently.

Review followups

• castVote and castVeto should return vote weight
• Refactor balance variable to be weight and wrap in utils once
• Remove hardcoded return boolean
• Remove weight == 0 check

2/3 voting period 1/3 confirmation period should be able to be updated

We should stress test assumptions here a bit more and make sure we're confident with this split.

Use minimal proxies for token caster and creator contracts and ensure the factory has deterministic deployments

We need to determine the salt. We can't use (msg.sender, token) but because then the executor couldn't be used to deploy.

test: migrate bySig and token voting factory tests

Migrate from deprecated repo https://github.com/llamaxyz/llama-periphery-deprecated

Verify all EIP-712 hashes

Currently from skimming I don't think the Domain Hash is unique if there can be multiple token voting modules per LlamaInstance. Since we're only using LlamaCore.name()

add sanity checks for token in adapter contract

https://github.com/spearbit-audits/review-llama-token-governor/pull/3#discussion_r1431359202

At deploy time check that clockAdapter was implemented correctly

Check ERC-165 supports interface.

Should we add a view function to get the caster address from the creator contract? It would be nice to link them together

Remove requirement that token voting contracts must be deployed from instance's executor

We seem to be using a lot of the strategy view functions for getting action state and other information

I thought we had decided that we wouldn't use strategy view functions directly since it can give outdated information and can change depending on the strategy implementation.

We should fallback to using getActionState or other functions in the core to derive information.

For example:

if (!actionInfo.strategy.isActionApproved(actionInfo)) revert ActionNotApproved();
if (actionInfo.strategy.isActionExpired(actionInfo)) revert ActionExpired();

in _castDisapproval should just be a check to getActionState checking that the current state is Queued.

Prefix all token voting contracts with `Llama`

Clock mode improvements

• Standardize ILlamaTokenClockAdapter to have clock() function that returns current timepoint
• ^ Check if this should be uint48 instead of uint256?
• _currentTimepointMinusOne uses clock() from adapter if _isClockModeTimestamp is false
• Add a CLOCK_MODE function to ILlamaTokenClockAdapter
• Change _getClockMode to get the clock mode from token.CLOCK_MODE(). If that fails get CLOCK_MODE from the adapter. If that fails or if there's no adapter default to timestamp clock mode
• _timestampToTimepoint should take the action creation time and subtract 1 after the conversion (i.e _timestampToTimepoint(action.creationTime - 1) should be _timestampToTimepoint(action.creationTime) - 1)

We should use `IStrategy` instead of `ILlamaRelativeStrategyBase`

We currently use the ILlamaRelativeStrategyBase for getting the queuingPeriod and approvalPeriod. We should just add these to the IStrategy even though it's not part of the canonical strategy interface. It's because TokenVoting depends on having a strategy which has a queuingPeriod and approvalPeriod (Which could be the absolute strategies too)

Change all `block.timestamp` references to `block.timestamp - 1`

And we should do this uniformly for ERC20 and ERC721 ActionCreator and Caster contracts.

We have multiple places where we use block.timestamp vs block.timestamp - 1. Here are a few of them:

1. During initialize should supply check be at block.timestamp or block.timestamp - 1?

2. During action creation, should balance check be at block.timestamp or block.timestamp - 1? The Llama Framework checks for permissions at block.timestamp. But the TokenVotingActionCreator checks at block.timestamp -1.We should discuss the effects of this including the fact that someone can't be asssigned a voting balance and createAction in the same block. Are there any negative issues due to this?

3. setActionThreshold also checks at block.timestamp - 1.

Architect contracts that a future block number based voting contract could inherit and override CLOCK_MODE

Lots more tests!! and integration tests with popular tokens like ENS, UNI, COMP, Nouns etc.

We should double check all the `>, >=, <, <=` comparisons for edge cases

Timestamps, quantities, balances, etc that are compared should be double checked for edge cases.

Remove the concept of ERC20 and ERC721 from the code.

It should just depend on Votes which both ERC20Votes and ERC721Votes inherit from.

We should hardcode submodule commits like we do in the main repo

`_insert` in the two checkpoints returns wrong values

If you compare the return values of _insert in the two checkpoints in this repo with the two checkpoints in the Llama repo, you can see that it's doing something else.

Either remove the returns or fix the returns.

Optimise `castData` struct

Why did we chose uint96 for the quantities?

Pass in `PeriodPcts` at deploy time.

#66 (comment)

During action creation, should balance check be at `block.timestamp` or `block.timstamp - 1`

The Llama Framework checks for permissions at block.timestamp. But the TokenVotingActionCreator checks at block.timestamp -1.

We should discuss the effects of this including the fact that someone can't be asssigned a voting balance and createAction in the same block. Are there any negative issues due to this?

Pre-audit checklist

• Can remove DuplicateSubmission since underlying DuplicateCast will catch it.
• Getters for mapping in CastData struct.
• Cast Vote and Cast Veto should check if the ActionCaster has the defined role still since that role can be removed and we won't know until the (Dis)approval has been submitted.
• Submit Approvals and Disapprovals should also have Action State checks.
• We should do vote weight adding similar to how we do _newCastCount in LlamaCore which has an upperbound condition.
• #45
• #32
• Handle missing natspec and ensure existing natspec is correct.
• #24
• #30
• comp like token support?
• Should we use uint48 for timepoint?
• #47
• LlamaTokenVotingLens contract to compute ActionCreator and Caster addresses.
• #28
• Check if terminology, function params (+ordering) and event params(+ ordering) match closely with OZ Governor.
• Change deploy function name on Factory from deployTokenVotingModule to deploy
• #51

Rename `test_RevertsIf` to `test_RevertIf`

This keeps naming consistent with the main llama repo and standardizes test naming in this repo.

fix: remove Llama-specific language from token voting module mechanics

To make our token voting module easy to understand, we should use language that's consistent with the mainstream governance frameworks. Instead of approval, we should use vote. Instead of threshold, we should use quorum. Rather than MIN_APPROVAL_PCT and MIN_DISAPPROVAL_PCT, we should use VOTE_QUORUM and VETO_QUORUM.

More renaming suggestions here: https://github.com/llamaxyz/llama-internal/pull/6#discussion_r1405144501

CLOCK_MODE refactor 2

• Change _getClockMode to get the clock mode from token.CLOCK_MODE(). If that fails get CLOCK_MODE from the adapter. If that fails or if there's no adapter default to timestamp clock mode

`submitApprovals` does not have a `checkIfAapprovalEnabled` check.

`checkIf(Dis)ApprovalEnabled` should use `address(this)` instead of caster/tokenholder.

https://github.com/llamaxyz/llama-periphery/blob/main/src/token-voting/TokenholderCaster.sol#L329

If you take a look at how actionInfo.strategy.checkIfApprovalEnabled(actionInfo, caster, role); is defined in the strategy, it expects the policyholder instead of caster though it doesn't do anything with it right now. But a future strategy implementation might and we should stick to passing in data that it expects.

This means we should pass in address(this) since that's the policyholder and not the tokenholder.

There are multiple occurrences of this in both the ERC20 and ERC721 caster. We should fix all of them.

Use different salt for deterministic deployments of minimal proxies.

We currently use (msg.sender, token) . But we might want to deploy multiple token voting modules for the same token and executor (Eg: with different approval pct. etc.)

Consider a different salt. Adding a nonce to the salt is one option (but explore other options too).

Use Immutable ROLE variable in ActionCreator contract

In the caster we are using an immutable role, but not for the action creator.
If we were to make this immutable, we could remove the need for the user to pass in the role when creating an action which will improve UX.

Post-audit checklist

• LlamaTokenVotingLens contract to compute LlamaTokenGovernor address.
• #47
• Handle missing natspec and ensure existing natspec is correct.
• #80
• Create llama script that creates the roles, deploys the token voting module, and assigns them. The role name can be formatted like UNI token proposer or just Token Proposer if only one governance token is expected

Uints cannot be less than 0 for `InvalidVoteQuorumPct` check

We can change:

    if (_voteQuorumPct > ONE_HUNDRED_IN_BPS || _voteQuorumPct <= 0) revert InvalidVoteQuorumPct(_voteQuorumPct);
    if (_vetoQuorumPct > ONE_HUNDRED_IN_BPS || _vetoQuorumPct <= 0) revert InvalidVetoQuorumPct(_vetoQuorumPct);

to

    if (_voteQuorumPct > ONE_HUNDRED_IN_BPS || _voteQuorumPct == 0) revert InvalidVoteQuorumPct(_voteQuorumPct);
    if (_vetoQuorumPct > ONE_HUNDRED_IN_BPS || _vetoQuorumPct == 0) revert InvalidVetoQuorumPct(_vetoQuorumPct);

https://github.com/spearbit-audits/review-llama-token-governor/pull/1#discussion_r1430517733

ILlamaTokenAdapter

• CLOCK_MODE refactor 2
• Change _getClockMode to get the clock mode from token.CLOCK_MODE(). If that fails get CLOCK_MODE from the adapter. If that fails or if there's no adapter default to timestamp clock mode
• Turn clock adapter to general token adapter and add getPastVotes and getPastTotalSupply functions
• Define mode=timestamp as DEFAULT_CLOCK_MODE constant