Skip to content

Commit

Permalink
fix: pre-review vulnerability fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@satello
satello committed Nov 27, 2018
1 parent 05e04d5 commit b58a1ca
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 23 deletions.
43 changes: 24 additions & 19 deletions contracts/standard/arbitration/MultipleArbitrableTransaction.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ contract MultipleArbitrableTransaction {
uint buyerFee; // Total fees paid by the buyer.
uint lastInteraction; // Last interaction for the dispute procedure.
Status status;
uint arbitrationCost;
}

Transaction[] public transactions;
Expand Down Expand Up @@ -105,7 +106,7 @@ contract MultipleArbitrableTransaction {
}

/** @dev Create a transaction.
* @param _timeoutPayment Time after which a party automatically loose a dispute.
* @param _timeoutPayment Time after which a party automatically loses the dispute.
* @param _seller The recipient of the transaction.
* @param _metaEvidence Link to the meta-evidence.
* @return The index of the transaction.
Expand All @@ -124,7 +125,8 @@ contract MultipleArbitrableTransaction {
sellerFee: 0,
buyerFee: 0,
lastInteraction: now,
status: Status.NoDispute
status: Status.NoDispute,
arbitrationCost: 0
}));
emit MetaEvidence(transactions.length - 1, _metaEvidence);
return transactions.length - 1;
Expand Down Expand Up @@ -163,7 +165,7 @@ contract MultipleArbitrableTransaction {
*/
function executeTransaction(uint _transactionId) public {
Transaction storage transaction = transactions[_transactionId];
require(now >= transaction.lastInteraction + transaction.timeoutPayment, "The timeout has not passed yet.");
require(now - transaction.lastInteraction >= transaction.timeoutPayment, "The timeout has not passed yet.");
require(transaction.status == Status.NoDispute, "The transaction can't be disputed.");

transaction.seller.transfer(transaction.amount);
Expand All @@ -179,7 +181,7 @@ contract MultipleArbitrableTransaction {
Transaction storage transaction = transactions[_transactionId];

require(transaction.status == Status.WaitingSeller, "The transaction is not waiting on the seller.");
require(now >= transaction.lastInteraction + timeoutFee, "Timeout time has not passed yet.");
require(now - transaction.lastInteraction >= + timeoutFee, "Timeout time has not passed yet.");

executeRuling(_transactionId, BUYER_WINS);
}
Expand All @@ -191,7 +193,7 @@ contract MultipleArbitrableTransaction {
Transaction storage transaction = transactions[_transactionId];

require(transaction.status == Status.WaitingBuyer, "The transaction is not waiting on the buyer.");
require(now >= transaction.lastInteraction + timeoutFee, "Timeout time has not passed yet.");
require(now - transaction.lastInteraction >= timeoutFee, "Timeout time has not passed yet.");

executeRuling(_transactionId, SELLER_WINS);
}
Expand All @@ -202,6 +204,7 @@ contract MultipleArbitrableTransaction {
*/
function payArbitrationFeeByBuyer(uint _transactionId) public payable {
Transaction storage transaction = transactions[_transactionId];
require(transaction.status < Status.DisputeCreated, "Dispute has already been created.");
require(msg.sender == transaction.buyer, "The caller must be the buyer.");

uint arbitrationCost = arbitrator.arbitrationCost(arbitratorExtraData);
Expand All @@ -215,8 +218,6 @@ contract MultipleArbitrableTransaction {
transaction.status = Status.WaitingSeller;
emit HasToPayFee(_transactionId, Party.Seller);
} else { // The buyer has also paid the fee. We create the dispute
// Make sure a dispute has not been created yet.
require(transaction.status < Status.DisputeCreated, "Dispute has already been created.");
raiseDispute(_transactionId, arbitrationCost);
}
}
Expand All @@ -228,6 +229,7 @@ contract MultipleArbitrableTransaction {
*/
function payArbitrationFeeBySeller(uint _transactionId) public payable {
Transaction storage transaction = transactions[_transactionId];
require(transaction.status < Status.DisputeCreated, "Dispute has already been created.");
require(msg.sender == transaction.seller, "The caller must be the seller.");

uint arbitrationCost = arbitrator.arbitrationCost(arbitratorExtraData);
Expand All @@ -241,8 +243,6 @@ contract MultipleArbitrableTransaction {
transaction.status = Status.WaitingBuyer;
emit HasToPayFee(_transactionId, Party.Buyer);
} else { // The seller has also paid the fee. We create the dispute
// Make sure a dispute has not been created yet.
require(transaction.status < Status.DisputeCreated, "Dispute has already been created.");
raiseDispute(_transactionId, arbitrationCost);
}
}
Expand All @@ -255,6 +255,7 @@ contract MultipleArbitrableTransaction {
Transaction storage transaction = transactions[_transactionId];
transaction.status = Status.DisputeCreated;
transaction.disputeId = arbitrator.createDispute.value(_arbitrationCost)(AMOUNT_OF_CHOICES, arbitratorExtraData);
transaction.arbitrationCost = _arbitrationCost;
disputeTxMap[keccak256(arbitrator, transaction.disputeId)] = _transactionId;
emit Dispute(arbitrator, transaction.disputeId, _transactionId);
}
Expand Down Expand Up @@ -291,6 +292,7 @@ contract MultipleArbitrableTransaction {
uint transactionId = disputeTxMap[keccak256(msg.sender, _disputeID)];
Transaction storage transaction = transactions[transactionId];
require(msg.sender == address(arbitrator), "The caller must be the arbitrator.");
require(transaction.status < Status.Resolved, "The dispute has already been resolved.");

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

Expand All @@ -305,24 +307,27 @@ contract MultipleArbitrableTransaction {
Transaction storage transaction = transactions[_transactionId];
require(_ruling <= AMOUNT_OF_CHOICES, "Invalid ruling.");

uint maxFee = transaction.sellerFee > transaction.buyerFee ? transaction.sellerFee : transaction.buyerFee;

// Give the arbitration fee back.
// Note that we use send to prevent a party from blocking the execution.
if (_ruling == SELLER_WINS) {
// 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(maxFee);
transaction.seller.send(transaction.amount);
transaction.seller.send(transaction.sellerFee + transaction.amount);
// Refund buyer if they overpaid
if (transaction.buyerFee > transaction.arbitrationCost) // It should be impossible for aritrationCost to be greater than fee but extra check here to prevent underflow.
transaction.buyer.send(transaction.buyerFee - transaction.arbitrationCost);
} else if (_ruling == BUYER_WINS) {
transaction.buyer.send(maxFee);
transaction.buyer.send(transaction.amount);
transaction.buyer.send(transaction.buyerFee + transaction.amount);
// Refund seller if they overpaid
if (transaction.sellerFee > transaction.arbitrationCost) // It should be impossible for aritrationCost to be greater than fee but extra check here to prevent underflow.
transaction.seller.send(transaction.sellerFee - transaction.arbitrationCost);
} else {
uint split_anount = (maxFee + transaction.amount) / 2;
transaction.buyer.send(split_anount);
transaction.seller.send(split_anount);
uint split_amount = (transaction.sellerFee + transaction.buyerFee - transaction.arbitrationCost + transaction.amount) / 2;
transaction.buyer.send(split_amount);
transaction.seller.send(split_amount);
}

transaction.amount = 0;
transaction.sellerFee = 0;
transaction.buyerFee = 0;
transaction.status = Status.Resolved;
}

Expand Down
54 changes: 50 additions & 4 deletions test/multiple-arbitrable-transaction.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -489,18 +489,64 @@ contract('MultipleArbitrableTransaction', function(accounts) {
const payeeBalanceBeforeRuling = web3.eth.getBalance(payee)

await centralizedArbitrator.giveRuling(0, 0, { from: arbitrator })

const payerBalanceAfterRuling = web3.eth.getBalance(payer)
const payeeBalanceAfterRuling = web3.eth.getBalance(payee)

assert.equal(
payeeBalanceAfterRuling.toString(),
payeeBalanceBeforeRuling.plus(510).toString(),
'The payee has not been reimbursed correctly'
)

assert.equal(
payerBalanceAfterRuling.toString(),
payerBalanceBeforeRuling.plus(510).toString(),
'The payee has not been paid properly'
'The payer has not been paid properly'
)
})

it('Should refund overpaid arbitration fee for payer', async () => {
const centralizedArbitrator = await CentralizedArbitrator.new(
arbitrationFee,
{ from: arbitrator }
)
const multipleContract = await MultipleArbitrableTransaction.new(
centralizedArbitrator.address,
0x0,
0,
{ from: payer }
)

const lastTransaction = await getLastTransaction(
multipleContract,
async () => {
await multipleContract.createTransaction(
timeoutPayment,
payee,
metaEvidenceUri,
{ from: payer, value: amount }
)
}
)
const arbitrableTransactionId = lastTransaction.args._metaEvidenceID.toNumber()

const extraAmount = 100
await multipleContract.payArbitrationFeeByBuyer(arbitrableTransactionId, {
from: payer,
value: arbitrationFee + extraAmount
})
await multipleContract.payArbitrationFeeBySeller(arbitrableTransactionId, {
from: payee,
value: arbitrationFee
})
const payerBalanceBeforePay = web3.eth.getBalance(payer)
await centralizedArbitrator.giveRuling(0, 2, { from: arbitrator })
const newPayerBalance = web3.eth.getBalance(payer)
assert.equal(
payeeBalanceAfterRuling.toString(),
payeeBalanceBeforeRuling.plus(510).toString(),
'The payer has not been reimbursed correctly'
newPayerBalance.toString(),
payerBalanceBeforePay.plus(extraAmount).toString(),
'The payer was not refunded properly'
)
})

Expand Down

0 comments on commit b58a1ca

Please sign in to comment.