Pre-Review Appealable Arbitrable Library

[nix1g]

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.

[fnanni-0]

@nix1g thanks for the review, sorry that it took me a while to answer. Still work in progress but I just pushed most of the changes here

L143: verify that the dispute exists as well.

In some cases this might be important, but I'll rather delegate that responsibility to the user. Also, AFAIK we don't check that in none of our contracts.

• DisputeData refactor: great idea, I used it.
• Using local variables: done.
• require: I cut error messages down to 32 bytes and merged some of them into one. I'm not sure about removing them entirely.
• Miscellaneous
  1. Done.
  2. Done.
  3. Good point! I removed it.
  4. Because it's only 250 I think I'll rather l prioritize simplicity.
  5. I don't understand what you would change here.
  6. Done.
  7. appealCost could theoretically be an unpayable amount. I guess that's not going to happen when using Kleros courts, but still I'm not sure it's a good idea to remove CappedMath.
• Other: done.
• Readme: nice, thanks. It wasn't working on the VSC preview so I thought I had to use js.

Here you can see the gas difference of using the old and the new binary library (with the escrow mock). Look at batch withdrawals, that's crazy! The extra gas needed in rule() goes away if I use a mapping instead of an array for rounds.

Extra: I changed all the constants from public to private. I don't think we need getters for those.

[fnanni-0]

BinaryArbitrable.sol and MultiOutcome.sol are close, it would be great if they were consistent (make a diff).

What do you mean with "make a diff"?

Lastly, there is no test file for this library.

this one

[nix1g]

I left some comments on the commit (also you can reference an issue in a commit message), everything else is OK.

5. The condition totalFeesPaid > 0 and paidFees > 0 are false only if round.feeRewards == 0, in which case there is nothing to withdraw anyway so a div-by-zero exception here is harmless. I would remove the if/?: altogether. IMO the ideal alternative is to bypass solidity's div-by-zero check to get the same behavior (since the DIV opcode returns 0 when the divisor is 0) but that requires (very little) assembly so I did not propose.

6. I forgot about NON_PAYABLE_AMOUNT my bad

Using a mapping instead of an array for DisputeData.rounds is a good idea as in most cases there is only one round, so storing the last index instead of the length saves gas in this case.

Extra: for an internal library there must be no public or external so yeah good to remove.

BinaryArbitrable.sol and MultiOutcome.sol are close, it would be great if they were consistent (make a diff).

What do you mean with "make a diff"?

To check the differences between two files, you can compare them using diff. That way you can easily spot discrepancies and reviewers using this tool have a clear understanding of the difference between files. Check out the wiki, the man or this online tool.

Lastly, there is no test file for this library.

this one

I must be blind :see_no_evil:

[fnanni-0]

The condition totalFeesPaid > 0 and paidFees > 0 are false only if round.feeRewards == 0, in which case there is nothing to withdraw anyway so a div-by-zero exception here is harmless

The problem with this is that it affects batch withdrawing as well and the exception wouldn't be ok there. I couldn't think of a way to make this simpler.

To check the differences between two files, you can compare them using diff. That way you can easily spot discrepancies and reviewers using this tool have a clear understanding of the difference between files. Check out the wiki, the man or this online tool.

Ah I see now. I'll give it a try.