Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added MultipleArbitrableTransaction.sol and fixed issue #27 #32

Merged
merged 8 commits into from
Jun 29, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
320 changes: 320 additions & 0 deletions contracts/standard/arbitration/MultipleArbitrableTransaction.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,320 @@
/**
* @title Multiple Arbitrable Transaction
* Bug Bounties: This code hasn't undertaken a bug bounty program yet.
*/


pragma solidity ^0.4.15;
import "./Arbitrator.sol";

/** @title Multiple Arbitrable Transaction
* This is a a contract for multiple arbitrated transactions which can be reversed by an arbitrator.
* This can be used for buying goods, services and for paying freelancers.
* Parties are identified as "seller" and "buyer".
*/
contract MultipleArbitrableTransaction {
string constant RULING_OPTIONS = "Reimburse buyer;Pay seller";



uint8 constant AMOUNT_OF_CHOICES = 2;
uint8 constant BUYER_WINS = 1;
uint8 constant SELLER_WINS = 2;

enum Party {Seller, Buyer}

enum Status {NoDispute, WaitingSeller, WaitingBuyer, DisputeCreated, Resolved}

struct Transaction {
address seller;
address buyer;
uint256 amount;
uint256 timeout; // Time in seconds a party can take before being considered unresponding and lose the dispute.
uint disputeId;
bool disputed;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disputed seems redundant with status, I think we can remove disputed

Arbitrator arbitrator;
bytes arbitratorExtraData;
uint sellerFee; // Total fees paid by the seller.
uint buyerFee; // Total fees paid by the buyer.
uint lastInteraction; // Last interaction for the dispute procedure.
Status status;
}

Transaction[] transactions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be public.


mapping (bytes32 => uint) disputeTxMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also be public.


/** @dev Constructor.
*/
function MultipleArbitrableTransaction() public {
}

/** @dev To be raised when a dispute is created. The main purpose of this event is to let the arbitrator know the meaning ruling IDs.
* @param _transactionId The index of the transaction in dispute.
* @param _arbitrator The arbitrator of the contract.
* @param _disputeID ID of the dispute in the Arbitrator contract.
* @param _rulingOptions Map ruling IDs to short description of the ruling in a CSV format using ";" as a delimiter. Note that ruling IDs start a 1. For example "Send funds to buyer;Send funds to seller", means that ruling 1 will make the contract send funds to the buyer and 2 to the seller.
*/
event Dispute(uint indexed _transactionId, Arbitrator indexed _arbitrator, uint indexed _disputeID, string _rulingOptions);

/** @dev To be raised when a ruling is given.
* @param _transactionId The index of the transaction in dispute.
* @param _arbitrator The arbitrator giving the ruling.
* @param _disputeID ID of the dispute in the Arbitrator contract.
* @param _ruling The ruling which was given.
*/
event Ruling(uint indexed _transactionId, Arbitrator indexed _arbitrator, uint indexed _disputeID, uint _ruling);

/** @dev To be raised when evidence are submitted. Should point to the ressource (evidences are not to be stored on chain due to gas considerations).
* @param _transactionId The index of the transaction.
* @param _arbitrator The arbitrator of the contract.
* @param _disputeID ID of the dispute in the Arbitrator contract.
* @param _party The address of the party submiting the evidence. Note that 0 is kept for evidences not submitted by any party.
* @param _evidence A link to evidence or if it is short the evidence itself. Can be web link ("http://X"), IPFS ("ipfs:/X") or another storing service (using the URI, see https://en.wikipedia.org/wiki/Uniform_Resource_Identifier ). One usecase of short evidence is to include the hash of the plain English contract.
*/
event Evidence(uint indexed _transactionId, Arbitrator indexed _arbitrator, uint indexed _disputeID, address _party, string _evidence);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evidence is defined in Arbitrable https://github.com/kleros/kleros-interaction/blob/master/contracts/standard/arbitration/Arbitrable.sol#L45
To keep consistency, it's better to use the same evidence event (without the _transactionID which can be recovered form _party and _disputeID).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clesaege _party and _disputeID does not make it unique in this contract because we can have 2 transactions with the same party and different arbitrators. That's why I brought Evidence event to this contract instead of using the Arbitrable one. Please let me know what to do regarding this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but _disputeID and _arbitrator makes it unique.
ethereum/EIPs#792 : It must track the disputes by their (arbitrator,disputeID) unique key

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right @clesaege . I have made the requested changes now.


/** @dev To be emmited at contract creation. Contains the hash of the plain text contract. This will allow any party to show what was the original contract.
* This event is used as cheap way of storing it.
* @param _transactionId The index of the transaction.
* @param _contractHash Keccak256 hash of the plain text contract.
*/
event ContractHash(uint indexed _transactionId, bytes32 _contractHash);

/** @dev Indicate that a party has to pay a fee or would otherwise be considered as loosing.
* @param _transactionId The index of the transaction.
* @param _party The party who has to pay.
*/
event HasToPayFee(uint indexed _transactionId, Party _party);

/** @dev Give a ruling for a dispute. Must be called by the arbitrator.
* The purpose of this function is to ensure that the address calling it has the right to rule on the contract.
* @param _disputeID ID of the dispute in the Arbitrator contract.
* @param _ruling Ruling given by the arbitrator. Note that 0 is reserved for "Not able/wanting to make a decision".
*/
function rule(uint _disputeID, uint _ruling) public {
uint transactionId = disputeTxMap[keccak256(msg.sender,_disputeID)];
Transaction storage transaction = transactions[transactionId];
require(msg.sender==address(transaction.arbitrator));

Ruling(transactionId, Arbitrator(msg.sender),_disputeID,_ruling);

executeRuling(_disputeID,_ruling);
}

/** @dev Pay the arbitration fee to raise a dispute. To be called by the seller. UNTRUSTED.
* Note that the arbitrator can have createDispute throw, which will make this function throw and therefore lead to a party being timed-out.
* This is not a vulnerability as the arbitrator can rule in favor of one party anyway.
* @param _transactionId The index of the transaction.
*/
function payArbitrationFeeBySeller(uint _transactionId) payable {
Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.seller);


uint arbitrationCost = transaction.arbitrator.arbitrationCost(transaction.arbitratorExtraData);
transaction.sellerFee += msg.value;
require(transaction.sellerFee == arbitrationCost); // Require that the total pay at least the arbitration cost.
Copy link
Member

@clesaege clesaege Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be >= so it's at least. Otherwise it would block when the arbitrator lowers the fees. Note that it was a bug in the ArbitrableTransaction which is no fixed.

require(transaction.status < Status.DisputeCreated); // Make sure a dispute has not been created yet.

transaction.lastInteraction = now;
if (transaction.buyerFee < arbitrationCost) { // The partyB still has to pay. This can also happens if he has paid, but arbitrationCost has increased.
transaction.status = Status.WaitingBuyer;
HasToPayFee(_transactionId, Party.Buyer);
} else { // The partyB has also paid the fee. We create the dispute
raiseDispute(_transactionId, arbitrationCost);
}
}

/** @dev Pay the arbitration fee to raise a dispute. To be called by the buyer. UNTRUSTED.
* Note that this function mirror payArbitrationFeeBySeller.
* @param _transactionId The index of the transaction.
*/
function payArbitrationFeeByBuyer(uint _transactionId) payable {
Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.buyer);

uint arbitrationCost = transaction.arbitrator.arbitrationCost(transaction.arbitratorExtraData);
transaction.buyerFee += msg.value;
require(transaction.buyerFee == arbitrationCost); // Require that the total pay at least the arbitration cost.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for the seller one. == -> >=

require(transaction.status < Status.DisputeCreated); // Make sure a dispute has not been created yet.

transaction.lastInteraction = now;
if (transaction.sellerFee < arbitrationCost) { // The partyA still has to pay. This can also happens if he has paid, but arbitrationCost has increased.
transaction.status = Status.WaitingSeller;
HasToPayFee(_transactionId, Party.Seller);
} else { // The partyA has also paid the fee. We create the dispute
raiseDispute(_transactionId, arbitrationCost);
}
}

/** @dev Create a dispute. UNTRUSTED.
* @param _transactionId The index of the transaction.
* @param _arbitrationCost Amount to pay the arbitrator.
*/
function raiseDispute(uint _transactionId, uint _arbitrationCost) internal {
Transaction storage transaction = transactions[_transactionId];
transaction.status = Status.DisputeCreated;
transaction.disputeId = transaction.arbitrator.createDispute.value(_arbitrationCost)(AMOUNT_OF_CHOICES,transaction.arbitratorExtraData);
transaction.disputed = true;
disputeTxMap[keccak256(transaction.arbitrator, transaction.disputeId)] = _transactionId;
Dispute(_transactionId, transaction.arbitrator, transaction.disputeId,RULING_OPTIONS);
}

/** @dev Reimburse partyA if partyB fails to pay the fee.
* @param _transactionId The index of the transaction.
*/
function timeOutBySeller(uint _transactionId) {
Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.seller);


require(transaction.status==Status.WaitingBuyer);
require(now>=transaction.lastInteraction+transaction.timeout);

executeRuling(transaction.disputeId, SELLER_WINS);
}

/** @dev Pay partyB if partyA fails to pay the fee.
* @param _transactionId The index of the transaction.
*/
function timeOutByBuyer(uint _transactionId) {
Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.buyer);


require(transaction.status == Status.WaitingSeller);
require(now>=transaction.lastInteraction+transaction.timeout);

executeRuling(transaction.disputeId,BUYER_WINS);
}

/** @dev Submit a reference to evidence. EVENT.
* @param _transactionId The index of the transaction.
* @param _evidence A link to an evidence using its URI.
*/
function submitEvidence(uint _transactionId, string _evidence) {
Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.buyer || msg.sender == transaction.seller);

require(transaction.status>=Status.DisputeCreated);
Evidence(_transactionId, transaction.arbitrator,transaction.disputeId,msg.sender,_evidence);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the _transactionId which will make it consistent with other Arbitrable contracts.

}

/** @dev Appeal an appealable ruling.
* Transfer the funds to the arbitrator.
* Note that no checks are required as the checks are done by the arbitrator.
* @param _transactionId The index of the transaction.
* @param _extraData Extra data for the arbitrator appeal procedure.
*/
function appeal(uint _transactionId, bytes _extraData) payable public {
Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.buyer || msg.sender == transaction.seller);

transaction.arbitrator.appeal.value(msg.value)(transaction.disputeId,_extraData);
}



/** @dev
* @param _arbitrator The arbitrator of the contract.
* @param _hashContract Keccak hash of the plain English contract.
* @param _timeout Time after which a party automatically loose a dispute.
* @param _seller The recipient of the transaction.
* @param _arbitratorExtraData Extra data for the arbitrator.
*/
function createTransaction(Arbitrator _arbitrator, bytes32 _hashContract, uint _timeout, address _seller, bytes _arbitratorExtraData) payable public returns (uint transactionIndex) {
transactions.push(Transaction({
seller: _seller,
buyer: msg.sender,
amount: msg.value,
timeout: _timeout,
disputed: false,
arbitrator: _arbitrator,
arbitratorExtraData: _arbitratorExtraData,
disputeId: 0,
sellerFee: 0,
buyerFee: 0,
lastInteraction: now,
status: Status.NoDispute
}));
ContractHash(transactions.length - 1, _hashContract);
return transactions.length - 1;
}

/** @dev
* returns the last transaction id whose buyer is the sender
*/
function lastTransactionId () constant public returns (uint index) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has a O(n) complexity and thus should only be used by the UI (we need to warn about that).
I also don't think this function is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clesaege I only use this in tests. Can I keep it? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a way to do without it? If so that would be better in order to lower code complexity (code ,complexity in tests is better than code complexity in smart contracts).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll figure it out

uint len = transactions.length;
for (uint i = len - 1; i>=0; i--) {
if (transactions[i].buyer == msg.sender) {
return i;
}
}
}

/** @dev
* @return amount The transaction amount. Can be called by buyer, seller or arbitrator.
* @param _transactionId The index of the transaction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just making transactions public?

*/
function amount (uint _transactionId) constant public returns (uint amount) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space after the function name

Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.buyer || msg.sender == transaction.seller || msg.sender == address(transaction.arbitrator));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This access control is not required because it's a constant function.
Note that everyone can read smart contracts variable so access control can only prevent state modification but never state read.


return transaction.amount;
}

/** @dev Transfer the transaction's amount to the seller if the timeout has passed
* @param _transactionId The index of the transaction.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a space before @param

*/
function withdraw(uint _transactionId) public {
Transaction storage transaction = transactions[_transactionId];
require(msg.sender == transaction.seller);
require(now>=transaction.lastInteraction+transaction.timeout);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to make sure it's not disputed, otherwise people could withdraw even if a dispute is raised.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require(transaction.status==Status.NoDispute);

transaction.seller.send(transaction.amount);
transaction.amount = 0;

transaction.status = Status.Resolved;
}

/** @dev Reimburse party A. To be called if the good or service can't be fully provided.
* @param _transactionId The index of the transaction.
* @param _amountReimbursed Amount to reimburse in wei.
*/
function reimburse(uint _transactionId, uint _amountReimbursed) public {
Transaction storage transaction = transactions[_transactionId];
require(transaction.seller == msg.sender);
require(_amountReimbursed <= transaction.amount);

transaction.buyer.transfer(_amountReimbursed);
transaction.amount -= _amountReimbursed;
}

/** @dev Execute a ruling of a dispute. It reimburse the fee to the winning party.
* This need to be extended by contract inheriting from it.
* @param _disputeID ID of the dispute in the Arbitrator contract.
* @param _ruling Ruling given by the arbitrator. 1 : Reimburse the partyA. 2 : Pay the partyB.
*/
function executeRuling(uint _disputeID, uint _ruling) internal {
uint transactionId = disputeTxMap[keccak256(msg.sender,_disputeID)];
Transaction storage transaction = transactions[transactionId];

require(_disputeID == transaction.disputeId);
require(_ruling <= AMOUNT_OF_CHOICES);

// Give the arbitration fee back.
// Note that we use send to prevent a party from blocking the execution.
if (_ruling == SELLER_WINS) {
transaction.seller.send(transaction.sellerFee > transaction.buyerFee ? transaction.sellerFee : transaction.buyerFee); // In both cases sends the highest amount paid to avoid ETH to be stuck in the contract if the arbitrator lowers its fee.
transaction.seller.send(transaction.amount);
} else if (_ruling == BUYER_WINS) {
transaction.buyer.send(transaction.sellerFee > transaction.buyerFee ? transaction.sellerFee : transaction.buyerFee);
transaction.buyer.send(transaction.amount);
}

transaction.amount = 0;
transaction.status = Status.Resolved;
}
}
4 changes: 2 additions & 2 deletions contracts/standard/arbitration/TwoPartyArbitrable.sol
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ contract TwoPartyArbitrable is Arbitrable {
function payArbitrationFeeByPartyA() payable onlyPartyA {
uint arbitrationCost=arbitrator.arbitrationCost(arbitratorExtraData);
partyAFee+=msg.value;
require(partyAFee == arbitrationCost); // Require that the total pay at least the arbitration cost.
require(partyAFee >= arbitrationCost); // Require that the total pay at least the arbitration cost.
require(status<Status.DisputeCreated); // Make sure a dispute has not been created yet.

lastInteraction=now;
Expand All @@ -82,7 +82,7 @@ contract TwoPartyArbitrable is Arbitrable {
function payArbitrationFeeByPartyB() payable onlyPartyB {
uint arbitrationCost=arbitrator.arbitrationCost(arbitratorExtraData);
partyBFee+=msg.value;
require(partyBFee == arbitrationCost); // Require that the total pay at least the arbitration cost.
require(partyBFee >= arbitrationCost); // Require that the total pay at least the arbitration cost.
require(status<Status.DisputeCreated); // Make sure a dispute has not been created yet.

lastInteraction=now;
Expand Down
Loading