kleros/kleros-interaction

[Bug Bounty: up to 50 ETH] Multiple Arbitrable Transaction

Closed this issue · 15 comments

n1c01a5 commented

Multiple Arbitrable Transactions Bounties

This is a bug bounty on the Multiple Arbitrable Transaction contract.
Bugs are rewarded up to 50 ETH according to this classification:

  • Critical Bugs: 50 ETH
    for bugs that enable stealing significant user funds.
  • Major Bugs: 25 ETH
    for bugs that can lock user funds or enable stealing a low amount (such as the fees) of them.
  • Minor Bugs: 5 ETH
    for smaller bugs.

If you found a bug you can send a mail to clement@kleros.io and nicolas@kleros.io.

Multiple Arbitrable Transactions

  • Sender makes an arbitrable transaction to a receiver. It can be automatically executed after _timeoutPayment.
  • The sender can have the contract pay (in part of totally) the amount using pay.
  • The receiver can have the contract reimburse (in part or totally) the sender by using reimburse.
  • Both parties can pay arbitration fees, giving some time to the other to pay the fees too to create a dispute. If one party fails to pay the fees, this party forfeits the amount.
  • Note that in case the arbitrator changes the fees after one party paid it, the burden of fee payment can make multiple back and forth. In practice, fees should not change that often and it should be an edge case. Extra fees due to over-payment or fee change are reimbursed.
  • The arbitrator which is ERC792 can rule disputes in favor of either party. The winning party gets the amount in the contract and is reimbursed the fees.
  • If the arbitrator "rules 0", the amount in the contract (initial value and remaining fees) is split within the parties (weis being trapped due to rounding are OK).

Bounty

Smart Contract Guidelines

We use those guidelines to write smart contracts. In particular, we do not try to prevent stupid behaviors at the contract level but leave this task to the UI. Letting the possibility to a user to harm itself is not a vulnerability (but should of course be dealt at the UI level).

Violation of guidelines are not vulnerabilities but can be reported as "suggestion for tips".

Bounty Rules

  • If you have any questions, don't hesitate to ask on the slack channel (slack.kleros.io #smart-contract-review) or by sending a mail to clement@kleros.io .
  • All this code is provided under MIT license and can be reused by other projects. If you don't hesitate to inform us and we may list your deployed contracts in the @deployed of the RAB pragma.
  • Good luck hunting and have fun hunting!
gitcoinbot commented

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 5.0 ETH (691.06 USD @ $138.21/ETH) attached to it as part of the @kleros fund.

gitcoinbot commented

Issue Status: 1. Open 2. Cancelled

Work has been started.

These users each claimed they can complete the work by 3 months, 1 week ago.
Please review their action plans below:

1) scammi has started work.

I'm reviewing you contract, I will contact the email with the report.

Thank you.
Santiago from Buenos Aires. Arg.
2) biuxmaster has started work.

I pretend to review this contract . I will contact the email with the report.
3) ajmachado has started work.

I would like to review the contract for security vulnerabilities.
4) l-kh has started work.

Hello, I am bug bounty hunter of solidity code. I have Master grade in finance engineering and this year I will start PhD in blockchain technology and Dapp.

Learn more on the Gitcoin Issue Details page.

gitcoinbot commented

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 5.0 ETH (1083.11 USD @ $216.62/ETH) has been submitted by:

  1. @biuxmaster
  2. @ajmachado
  3. @l-kh

@clesaege please take a look at the submitted work:

biuxmaster commented

Hi, I sent the report of my work via email for the 2 accounts that are on the description, I hope it's what you need

marsrobertson commented

  1. Deadline of the submissions? (make it ongoing)

  2. Add to the list: https://consensys.github.io/smart-contract-best-practices/bug_bounty_list/

Issues and PRs are welcome to add new bounties, or remove those which are no longer active.

https://github.com/ConsenSys/smart-contract-best-practices/blob/master/docs/bug_bounty_list.md

(assuming the bounty is ongoing)

More eyeballs = higher security.

At first I was overwhelmed by the complexity, after participating in the hackathon I got the understanding of what's going on but I'm a little bit tired... Need to allow some time to rest :)

clesaege commented

@biuxmaster What was your email title (we got reports but I don't know if it was from you, so just want to be sure I haven't missed any).

clesaege commented

@marsrobertson Found minor issue:
In case a party paid the fee, the fee increased, the second payed the fee, but the first one timed out, the first fee paid stayed locked in the contract forever.
This should have been fixed by 770cdda#diff-1784bdab8a238afa7c94981e245fd676
The bounty is paused for 3 days (for issues which would be related) to allow Kleros team to review those changes.

marsrobertson commented

I can confirm the issue is relatively minor, edge case, unlikely scenario, a few things would need to happen, I don’t think it would ever occur in real usage.

(nevertheless, bug is a bug, it shouldn't happen even if we assume it is unlikely)

The team responded quickly and provided fix ASAP.

🥚🥚🥚Happy hunting 🥚🥚🥚

L-KH commented

### Costly loop

Lines: 360-363
Lines: 369-372

Ethereum is a very resource-constrained environment. Prices per computational step are orders of magnitude higher than with centralized providers. Moreover, Ethereum miners impose a limit on the total number of gas consumed in a block. If array.length is large enough, the function exceeds the block gas limit, and transactions calling it will never be confirmed
for (uint256 i = 0; i < array.length ; i++) {
cosltyFunc();
}
This becomes a security issue, if an external actor influences array.length. E.g., if array enumerates all registered addresses, an adversary can register many addresses, causing the problem described above.

marsrobertson commented

@L-KH it's a view function at the bottom of the file.

https://github.com/kleros/kleros-interaction/blob/master/contracts/standard/arbitration/MultipleArbitrableTransaction.sol#L341-L343

It is meant to be called off-chain and then the result used for on-chain transactions, for the exact reasons you explained.

EDIT / UPDATE:

"consumes extra gas"
"more gas efficient"

These functions are called off-chain. No gas usage. No over block has limit.

L-KH commented

##Extra gas consumption

Lines: 369-372
Lines: 360-363

State variable, .balance, or .length is used in the condition of for or while loop. In this case, every iteration of loop consumes extra gas.

function getTransactionIDsByAddress(address _address) public view returns (uint[] transactionIDs) {
        uint count = 0;
        for (uint i = 0; i < transactions.length; i++) {
            if (transactions[i].sender == _address || transactions[i].receiver == _address)
                count++;
        }

If state variable, .balance, or .length is used several times, holding its value in a local variable is more gas efficient.

clesaege commented

@L-KH Gas consumption does not matter there. The comments warns that it should only be used by the UI and not by smart contracts. https://github.com/kleros/kleros-interaction/blob/master/contracts/standard/arbitration/MultipleArbitrableTransaction.sol#L353

clesaege commented

Follow up on this new issue as some code was changed: #273

gitcoinbot commented

Issue Status: 1. Open 2. Cancelled

The funding of 5.0 ETH (1073.81 USD @ $214.76/ETH) attached to this issue has been cancelled by the bounty submitter