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

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

Closed
n1c01a5 opened this issue Mar 22, 2019 · 15 comments
Closed

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

n1c01a5 opened this issue Mar 22, 2019 · 15 comments
Assignees
Labels

Comments

@n1c01a5
Copy link
Contributor

n1c01a5 commented Mar 22, 2019

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 [email protected] and [email protected].

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 [email protected] .
  • 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!
@n1c01a5 n1c01a5 added the Bounty 💰 Bounty label Mar 22, 2019
@n1c01a5 n1c01a5 pinned this issue Mar 22, 2019
@gitcoinbot
Copy link

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
Copy link

gitcoinbot commented Apr 8, 2019

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
Copy link

gitcoinbot commented Apr 23, 2019

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
Copy link

biuxmaster commented Apr 23, 2019

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
Copy link

  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
Copy link
Member

clesaege commented May 3, 2019

@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
Copy link
Member

@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
Copy link

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 🥚🥚🥚

@clesaege clesaege unpinned this issue Jun 25, 2019
@L-KH
Copy link

L-KH commented Jul 26, 2019

### 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
Copy link

marsrobertson commented Jul 27, 2019

@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
Copy link

L-KH commented Jul 29, 2019

##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
Copy link
Member

clesaege commented Aug 1, 2019

@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
Copy link
Member

clesaege commented Aug 1, 2019

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

@clesaege clesaege closed this as completed Aug 1, 2019
@gitcoinbot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants