Review MultipleArbitrableTransaction

[clesaege]

• Look at this https://github.com/kleros/kleros-interaction/blob/master/contracts/standard/arbitration/MultipleArbitrableTransaction.sol contract for bugs and vulnerabilities.
• Write a review on it which can be as short (no issues) or as long (complex vulnerabilities) as it is necessary.
• This task is made to be duplicated.

[clesaege]

According to @epiqueras this need a lot of changes, so if you haven't started, you can for changes to review.

[0xferit]

• Enums could be used instead of magic numbers.
• Require check in line 319 (require(_disputeID == transaction.disputeId, "Wrong dispute ID.");) may be unnecessary, I can't think of a situation it fails unless there is a hardware level error in calculations.
• In line 303, I would say _amountToBeReimbursed instead of _amountReimbursed which made me think it is a counter which tracks reimbursed amount so far.
• *BySeller *ByBuyer functions could be merged into one if the resulting function is not getting too messy. If it gets too messy, this is good.
• There are two notations for denoting parties, PartyA/PartyB and Seller/Buyer. Better to be consistent and choose one of them. I think Seller/Buyer is better.

[epiqueras]

https://docs.google.com/document/d/12b9gm6RnXMri7uanysjFDR7XuO5EeF8i-QZkdAw4VuE/edit?usp=sharing

[n1c01a5]

something seems buggy with the timeout, for example for this transaction the buyer 0x580b9ca15035b8c99bda7b959eab185b40b19704 raised a timeout but the eth are sent to the seller see https://kovan.etherscan.io/tx/0x85ee70e14a16d1e392a2cb2d2ff62f0658156356e9b2e3d488d0e574076c27cf

[epiqueras]

Yeah, time outs are broken, see the first "Major" point in my review.