code-423n4/contracts

Code4rena contracts

Code4rena contracts

Scripts

Proposing transfers

# fill out example file for your network
cp .env.example .env
# create a batch transfers JSON file
# propose it
yarn hardhat propose --network polygon --json scripts/proposals/example.json

contracts's Issues

Review: buy tokens twice (after a human error)

Impact

In the ArenaTokenSale contract you can update the whitelist with changeWhiteList()

If you would accidentally call this function with old values of the whitelist, the whitelisted amount will be reset in the contract.
Then token buyers who already bought their share can buy their share again.
Note: Calling changeWhiteList() again with old values is a human error, but this could happen when copy/pasting from a spreadsheet

Proof of Concept

contracts/contracts/ArenaTokenSale.sol

Lines 134 to 146 in 4a86c18

	function changeWhiteList(address[] memory _buyers, uint256[] memory _newTokenOutAmounts)
	external
	{
	require(msg.sender == owner() || msg.sender == saleRecipient, "TokenSale: not authorized");
	require(
	_buyers.length == _newTokenOutAmounts.length,
	"TokenSale: parameter length mismatch"
	);
	for (uint256 i = 0; i < _buyers.length; i++) {
	whitelistedBuyersAmount[_buyers[i]] = _newTokenOutAmounts[i];
	}
	}

contracts/contracts/ArenaTokenSale.sol

Line 93 in 4a86c18

whitelistedBuyersAmount[msg.sender] -= _tokenOutAmount;

Recommended Mitigation Steps

Track the amount of tokens bought

Merkle proof implementation is subject to tree extension attacks

The merkle proof code does not check the length of the proofs, and so it is susceptible to a tree-extension attack (or second-preimage attack).

While this same code has been used in other projects, the key difference here is that the leaf nodes (recipient addresses) have been chosen by wardens (potential adversaries), whereas in other projects the leaf nodes are addresses grabbed from historical blockchain data (not chosen by potential adversaries).

This means a malicious airdrop receiver could have given you a malicious bytes32 "address" which is actually its own merkle root for a tree of addresses that the attacker controls. If you used such a malicious address as a leaf in your merkle tree, it would allow the attacker to generate arbitrarily many valid proofs for addresses they control.

After computing your merkle tree and learning its depth -- and before deploying these contracts -- I recommend adding a require(merkleProof.length == treeDepth, 'invalid proof') just above this line.

Unit tests

we need unit tests for all of our contracts. These can be parallelized. If you want to help please assign yourself to one of these, branch off from master and create a PR.
Check out the example in test/TokenLock.spec.ts

• C4Governor.sol (does this need to be tested? at least the parameters should be checked to make sense given our airdrop and token sale pays out 20% immediately)
• C4TokenSale.sol (@MrToph #28 )
• C4Token.sol (@MrToph #31 )
• TokenLock.sol (@HickupHH3 #29 )
• RevokableTokenLock.sol (@Styj #23 )

All PRs should be made to the master branch.

Use latest version of OpenZeppelin and upgrade to new governor contract

Due to a bug in GovernorCompatibilityBravo, it would be advisable to upgrade to the latest version, deploy and migrate to a new governor contract.

Once deployed, submit a governance proposal with the following actions:

• timelock.grantRole(PROPOSER_ROLE, newGovernorAddress)
• timelock.revokeRole(PROPOSER_ROLE, oldGovernorAddress)

Review: Use Governor Bravo

"It might be useful to use Governor Bravo as this has more features (see https://medium.com/tally-blog/understanding-governor-bravo-69b06f1875da)"

Generate Governor contract

Claimers start out with not voting weight

OZ's ERC20Votes does not default the delegatee votes to the token owner and when claiming one starts out with no votes.
This can be an issue as claimers cannot vote on anything until they self-delegated.

Proposed solution: Self-delegate on claim, see #33

Review: decide about timelock controller

One of the attention points is where to store the funds of the sponsors, see:
https://discord.com/channels/810916927919620096/905119522296590418/912742834900205628

A suggestion by @HickupHH3 is to use a separate timelock contract:
https://discord.com/channels/810916927919620096/905119522296590418/912812168485933137

If that is used then the governor doesn't need an integrated timelock contract (i think)

contracts/contracts/ArenaGovernor.sol

Line 8 in 2624db1

import "@openzeppelin/contracts/governance/extensions/GovernorTimelockControl.sol";

Also the timelock contract needs to have some functions added to transfer funds.

DAO governance MVP plan outline

Code4rena DAO governance MVP high level:

• Set up and deploy contracts for vanilla Compound governance setup
• Test suite for contracts
• #7
• #5
• #6
• #4
• #8
• #2
• For proposals: setup a website or connect to an available website
• #3
• Be able to take a genesis block (with vesting)
• Conduct initial token sale to token purchasers

Let's use this thread to hash out details and start creating issues to stub out the work needed. This is my understanding of action items from the call:

• @zscole can you please link to and/or upload the reference contracts and deploy scripts you mentioned?
• @0xleastwood and @HickupHH3 to handle setting up contracts
• @MrToph to handle creating tests
• @gpersoon to contribute as needed to the above

Please review / comment / correct these lists.

C4 is light on contests this week, so maybe we can make a good run at this and get it done by next Monday.

What do you all think?

Set up vesting

Decide airdrop approach

Decide on chain (Polygon)

Review: Check merkeproof is set in claimTokens()

Impact

The merkeproof is set at a later moment in time via setMerkleRoot()
If claimTokens() would be used before the merkeproof is set, it wouldn't work
(although this is very unlikely in practice)

Proof of Concept

contracts/contracts/ArenaToken.sol

Lines 74 to 81 in 4a86c18

	function claimTokens(uint256 amount, bytes32[] calldata merkleProof) external {
	require(block.timestamp < claimPeriodEnds, "ArenaToken: Claim period ended");
	// we don't need to check that `merkleProof` has the correct length as
	// submitting a valid partial merkle proof would require `leaf` to map
	// to an intermediate hash in the merkle tree but `leaf` uses msg.sender
	// which is 20 bytes instead of 32 bytes and can't be chosen arbitrarily
	bytes32 leaf = keccak256(abi.encodePacked(msg.sender, amount));
	(bool valid, uint256 index) = MerkleProof.verify(merkleProof, merkleRoot, leaf);

contracts/contracts/ArenaToken.sol

Lines 134 to 138 in 4a86c18

	function setMerkleRoot(bytes32 _merkleRoot) external onlyOwner {
	require(merkleRoot == bytes32(0), "ArenaToken: Merkle root already set");
	merkleRoot = _merkleRoot;
	emit MerkleRootChanged(_merkleRoot);
	}

Tools Used

Recommended Mitigation Steps

Check merkeproof != 0 claimTokens()

Set up Tally

Tally is deployed on Polygon at https://alpha.withtally.com

They have a form there for submitting governance contracts for listing in the Tally interface.

Token distribution plan

Generate basic ERC-20 token

Integration test / deployment script

Set up vote interface

We're planning to use Compound Governance Boardroom for our proposal / voting interface.

Instructions for adding a project to Boardroom here:

https://docs.boardroom.info/adding-your-project/adding-your-project