kleros/kleros-interaction

Pre-Review ArbitrableTokenList

eccentricexit opened this issue · 20 comments

eccentricexit commented

I'm adding @epiqueras's suggestions for history purposes:

- *You're still setting disputed to false, when it should be a historic record of wether an agreement was ever disputed. You are also requiring that the last agreement was not disputed to create a new one. This might break internal logic.*
- Some param ordering is off.
- You can move the `lastAction == 0 push tokenID` to the `latestAgreementID != 0x0` block.
- Do we really need to clear `challengeReward`?
- latestAgreementID() is unnecessary.
- *You need to factorize ruling in favor of either party, it's repeated way too many times.*
- *In `itemsCount()`, `disputed` should not be exclusive.*
- *The fundDisputeCache should always be cleared, currently it's only cleared if it's the agreement's first contribution.*
- Valid rulings are already enforced higher up in the inheritance `<= choices`, I think, let's not repeat it.
clesaege commented

https://docs.google.com/document/d/1V4LWh2bz_NwhQpkApPia576OgLbF-RjGChLDsshiaBk/edit#

Three main points:

  • We need to simplify the contract greatly in order to limit security risks and security practice resource consumption.
  • Most of the issues are due to trying to make it fit in existing structures while writing specific code would have been more efficient.
  • People should not be able to chose their own meta evidence otherwise they can just put a "I win" kind of meta evidence.
0xferit commented

Review for 47245c0

  1. Typo in comment "Arbitrator did not rule of refused to rule."

    kleros-interaction/contracts/standard/permission/ArbitrableTokenList.sol

    Line 65 in 47245c0

    Other, // Arbitrator did not rule of refused to rule.

  2. Is it OK to prevent evidence submissions if round.appealed?

    kleros-interaction/contracts/standard/permission/ArbitrableTokenList.sol

    Lines 814 to 822 in 47245c0

    function submitEvidence(bytes32 _tokenID, string _evidence) external {
    Token storage token = tokens[_tokenID];
    Request storage request = token.requests[token.requests.length - 1];
    require(request.disputed, "The request is not disputed.");
    Round storage round = request.rounds[request.rounds.length - 1];
    require(!round.appealed, "Request already appealed.");
    emit Evidence(arbitrator, request.disputeID, msg.sender, _evidence);

  3. It can be a problem to uniquely identify a token with symbolURI I think, because it can be easily replaced. For example, someone can make multiple submissions for token X and only diference is symbolURI but those different uris has the same symbol so effectively they are duplicate submissions interpreted like original submissions.

    kleros-interaction/contracts/standard/permission/ArbitrableTokenList.sol

    Lines 249 to 257 in 47245c0

    bytes32 tokenID = keccak256(
    abi.encodePacked(
    _name,
    _ticker,
    _addr,
    _symbolURI,
    _networkID
    )
    );

  4. Is it intended to declare Party.Challenger as winner when currentRuling == RulingOption.Other?

    kleros-interaction/contracts/standard/permission/ArbitrableTokenList.sol

    Lines 780 to 790 in 47245c0

    function rule(uint _disputeID, uint _ruling) public onlyArbitrator {
    Party winner;
    Party loser;
    RulingOption currentRuling = RulingOption(arbitrator.currentRuling(_disputeID));
    if(currentRuling == RulingOption.Accept) {
    winner = Party.Requester;
    loser = Party.Challenger;
    } else {
    winner = Party.Challenger;
    loser = Party.Requester;
    }

  5. I can't see where token status is updated in fundPotDispute(...) but this function emits TokenStatusChange. Is it intended?

    kleros-interaction/contracts/standard/permission/ArbitrableTokenList.sol

    Lines 382 to 453 in 47245c0

    function fundPotDispute(bytes32 _tokenID, Party _side) external payable {
    require(
    _side == Party.Requester || _side == Party.Challenger,
    "Side must be either the requester or challenger."
    );
    Token storage token = tokens[_tokenID];
    require(
    token.status == TokenStatus.RegistrationRequested || token.status == TokenStatus.ClearingRequested,
    "Token does not have any pending requests."
    );
    Request storage request = token.requests[token.requests.length - 1];
    require(!request.disputed, "The request is already disputed.");
    // If a challenger placed a deposit, contributions must be done before the end of the arbitration fees waiting time.
    if(request.challengerDepositTime > 0)
    require(
    now - request.challengerDepositTime < arbitrationFeesWaitingTime,
    "The arbitration fees funding period of the first round has already passed."
    );
    // Add contributions to the side's pot.
    request.pot[uint(_side)] += msg.value;
    request.contributions[msg.sender][uint(_side)] += msg.value;
    if (msg.value > 0)
    emit Contribution(_tokenID, msg.sender, _side, msg.value);
    if(request.challengerDepositTime == 0) // If no party placed a challenge deposit, the caller is prefunding arbitration fees and stake. Stop here.
    return;
    // Calculate and update the total amount of fees required from each side.
    Round storage round = request.rounds[0];
    round.requiredForSide = calculateRequiredForSide(_tokenID, round.oldWinnerTotalCost);
    // Fund challenger side from his pot.
    uint contribution;
    uint amountStillRequired = round.requiredForSide[uint(Party.Challenger)] - round.paidFees[uint(Party.Challenger)];
    (contribution, request.pot[uint(Party.Challenger)]) = calculateContribution(
    request.pot[uint(Party.Challenger)],
    amountStillRequired
    );
    round.paidFees[uint(Party.Challenger)] += contribution;
    request.feeRewards += contribution;
    // Fund requester side from his pot.
    amountStillRequired = round.requiredForSide[uint(Party.Requester)] - round.paidFees[uint(Party.Requester)];
    (contribution, request.pot[uint(Party.Requester)]) = calculateContribution(
    request.pot[uint(Party.Requester)],
    amountStillRequired
    );
    round.paidFees[uint(Party.Requester)] += contribution;
    request.feeRewards += contribution;
    // Raise dispute if both sides are fully funded.
    if (round.paidFees[uint(Party.Requester)] >= round.requiredForSide[uint(Party.Requester)] &&
    round.paidFees[uint(Party.Challenger)] >= round.requiredForSide[uint(Party.Challenger)]) {
    uint arbitrationCost = arbitrator.arbitrationCost(arbitratorExtraData);
    request.disputeID = arbitrator.createDispute.value(arbitrationCost)(2, arbitratorExtraData);
    disputeIDToTokenID[request.disputeID] = _tokenID;
    request.disputed = true;
    request.rounds.length++;
    request.feeRewards -= arbitrationCost;
    emit TokenStatusChange(
    request.parties[uint(Party.Requester)],
    request.parties[uint(Party.Challenger)],
    _tokenID,
    token.status,
    request.disputed
    );
    }
    }

eccentricexit commented

Great finds!

It can be a problem to uniquely identify a token with symbolURI I think, because it can be easily replaced. For example, someone can make multiple submissions for token X and only diference is symbolURI but those different uris has the same symbol so effectively they are duplicate submissions interpreted like original submissions.

This would change the requests context from being "request for registration" and "clear request" to also having one for "update image".

  • 👍1
eccentricexit commented

Bug: Here, we the ID of the MetaEvidence should be:

     ? 2 * metaEvidenceUpdates
      : 2 * metaEvidenceUpdates + 1,

kleros-interaction/contracts/standard/permission/ArbitrableTokenList.sol

Line 388 in dfe4ac9

token.status == TokenStatus.RegistrationRequested

clesaege commented

Good catch @unknownunknown1 . I fix it.