kleros/arbitrable-contract-libraries

Smart contracts and libraries that help arbitrable contracts become appealable as well.

Kleros

Kleros core smart contracts.

Get Started

1. Clone this repo.
2. Run yarn to install dependencies and then yarn run build to compile the contracts.

Scripts

• yarn run prettify - Apply prettier to the entire project.
• yarn run lint:sol - Lint the entire project's .sol files.
• yarn run lint:js - Lint the entire project's .js files.
• yarn run lint:sol --fix - Fix fixable linting errors in .sol files.
• yarn run lint:js --fix - Fix fixable linting errors in .js files.
• yarn run lint - Lint the entire project's .sol and .js files.
• yarn test - Run the truffle tests.
• yarn run cz - Run commitizen.
• yarn run build - Compile contracts.

Contributing

See CONTRIBUTING.md.

Learn how to develop arbitrable and arbitrator contracts here.

arbitrable-contract-libraries's Issues

Pre-Review Arbitrable Contract Libraries

https://docs.google.com/document/d/165kb_mdyXbcRFaezpWI4ZbJ4LRybKXnf0Brom9aUp9Q/edit?usp=sharing

Pre-Review Appealable Arbitrable Library

Pre-Review @ a403fa5

General comments

This library will likely be used by many smart contracts, so gas optimization is particularly important.

By making this a regular inheritable smart contract rather than a library, some functions could be exposed by default, such as submitEvidence(), withdrawFeesAndRewards(), getters... while still being overwritable.

There is little practical difference between internal libraries and inheritable contracts. I think libraries should be reserved for extending existing functionalities (like CappedMath) and inheritable contracts should be used for adding new features (like a new struct with its own functionalities in this case).

EDIT: We should avoid exposing functions by default to force the implementer to consciously make the choice to expose them.
Using an internal library is a guarantee for reviewers that simplifies their task (interfaces are OK to inherit from though).
This follows the principle "composition over inheritance"

BinaryArbitrable.sol

I did not find any vulnerability.

Bug (harmless)

L143: verify that the dispute exists as well.

Gas optimization

Storage

The DisputeData structure does not need status as this can be inferred by other means. I tested this (see this patch) and got an overall reduction in gas consumption, especially in createDispute (20000 gas).

First, Status.None is simply rounds.length == 0.
Then, to differentiate between Status.Disputed and Status.Resolved we need to change the defintion of ruling:

• 0: No ruling, Status.Disputed or Status.None
• 1: Invalid/Refused to rule, Status.Resolved
• 2: Option 1, Status.Resolved
• 3: Option 2, Status.Resolved

require

Messages in requires should be (strictly) less than 32 bytes long. If they can be removed, that's even better.

See L145, L162, L204.

Using local variables

Solidity's optimizer is bad at reducing access to storage when using deep structures. In consequence:

• Using a local stack variable to cache a storage value saves a suprising amount of gas.
• Using a local storage variable for intermediate structures (like round) consumes gas (negligeable amount though).

The following variables should thus be cached on the stack:

L156: dispute.rounds.length - 1, round.rulingFunded, round.paidFees[_ruling]
L300: dispute.ruling
L315: round.feeRewards
L322: round.paidFees[dispute.ruling]

Miscellaneous

1. L159: because of L167, L168, L169 and L176, Solidity already enforces _ruling <= AMOUNT_OF_CHOICES, so it can be removed from the require
2. L183: remove subCap as round.paidFees[_ruling] >= totalCost >= appealCost
3. L267-L269: couldn't this computation be done off-chain? If invalid data is supplied, the function will revert anyway. Also does not handle potential overflows (not harmful)
4. L300: we could create an alternative _getWithdrawableAmount() that takes dispute as a storage parameter rather than its ID to save 250 gas
5. L318,L324: paidFees can only be 0 if appealCost is 0, in which case feeRewards is also 0
6. L328: replace if by else if (saves 853 gas)
7. L378: I suggest removing CappedMath here as these values are trusted (saves 200 gas and 100 bytes)

Other

1. L167: should add a test to enforce that subCap has an effect
2. L174: comment "in order not to not block the contract"
3. L378: superfluous parenthesis

MultiOutcomeArbitrable.sol

Notes:

• BinaryArbitrable.sol and MultiOutcome.sol are close, it would be great if they were consistent (make a diff).
• Also for this reason, my review of MultiOutcome.sol is superficial, most of what I said above applies here as well.
• Lastly, there is no test file for this library.

1. L17: This should be -1 rather than -2 (this was discussed for the realitio proxy already, there are 2^256 voting options including invalid, so that's 2^256-1 valid options the arbitrable needs to declare)
2. L17: this syntax is deprecated in v0.8, the preferred syntax is now type(uint256).max - 2
3. L239-L240: the intermediate variable is unnecessary

README.md

GitHub has support for Solidity in Markdown, you can tag code blocks with ```solidity rather than ```js.

Review of BinaryArbitrable

Here's my take on BinaryArbitrable.sol:

https://docs.google.com/document/d/1xNBdZjBn_Aco9ef0vvv2v53qP1K3Bbj2bUSe8UVQWJk/edit?usp=sharing