Disputers and expert committee should not receive or be refunded bonds after dismissed resolution

[hats-bug-reporter]

Github username: @bahurum
Submission hash (on-chain): 0x1398bad727b98575f754097c690fc0bd1e63666ab475956b31251410b77bb25f
Severity: medium

Description:
Description
When a resolution is dismissed by the court, bonds are sent to the expert committee and refunded to some disputers even if they shouldn't be since expert committee and disputers were proven wrong by the court ruling.

Attack Scenario

1. A claim is submitted by the committee
2. Alice and Bob both dispute the claim
3. The expert committee agrees with the dispute, considering the dispute from Alice valid and the dispute from Bob invalid. The Expert committee calls acceptDispute() with Alice's address in _disputersToRefund and Bob's address in _disputersToConfiscate. Bob's bonds go to the expert committee
4. Someone challenges the expert committee resolution
5. The court rules in favor of the challenger and against the dispute, and calls dismissResolution().
6. Alice can call reclaimBond() to get the bonds for her dispute refunded despite being wrong in disputing the claim

In this scenario both Alice and the Expert committee are wrong (final court judgement is against them) and yet Alice gets her bonds refunded and the Expert committee gains Bob's bonds.

Recommendation
The correct behavior would be to send the bonds for the dispute to a third party (for example the Hats governance) in the case the court dismisses the expert committee's resolution.

To do this, _confiscateDisputers() should not send the confiscated tokens to the expert committee in acceptDispute(). Define totalBondsToConfiscate as a state variable to be able to confiscate the tokens later in case the court executes the expert committee's resolution.

function _confiscateDisputers(
    IHATClaimsManager _vault,
    bytes32 _claimId,
    address[] calldata _disputersToConfiscate
) internal {
-   uint256 totalBondsToConfiscate;
+   totalBondsToConfiscate = 0;
    for (uint256 i = 0; i < _disputersToConfiscate.length; ) {
        totalBondsToConfiscate += disputersBonds[_disputersToConfiscate[i]][
            _vault
        ][_claimId];
        disputersBonds[_disputersToConfiscate[i]][_vault][_claimId] = 0;
        unchecked {
            ++i;
        }
    }

-   token.safeTransfer(expertCommittee, totalBondsToConfiscate);

    emit DisputersConfiscated(_vault, _claimId, _disputersToConfiscate);
}

Refunds should wait until the final decision of the court to be available.

function reclaimBond(IHATClaimsManager _vault, bytes32 _claimId) external {
    if (!bondClaimable[msg.sender][_vault][_claimId]) {
        // the bond is claimable if either
        // (a) it is not part of the curr

        IHATClaimsManager.Claim memory claim = _vault.getActiveClaim();

        if (
-           claim.claimId == _claimId &&  // claim must be not active anymore and periods passed
-           block.timestamp <
-           claim.createdAt + claim.challengePeriod + claim.challengeTimeOutPeriod
+           claim.claimId == _claimId
        ) {
            revert CannotClaimBond();
        }

...

}

If the expert committee's resolution is executed by the court, send the totalBondsToConfiscate amount stored to the expert committee.

function executeResolution(
    IHATClaimsManager _vault,
    bytes32 _claimId
)

...

    _vault.approveClaim(
        _claimId,
        resolution.bountyPercentage,
        resolution.beneficiary
    );

+   token.safeTransfer(expertCommittee, totalBondsToConfiscate);
    emit ResolutionExecuted(_vault, _claimId);
}

If the resolution is dismissed, the bonds should be sent to the chosen third party dismissedResolutionBondsReceiver

function dismissResolution(
    IHATClaimsManager _vault,
    bytes32 _claimId
)
    external
    onlyChallengedActiveClaim(_vault, _claimId)
    onlyResolvedDispute(_vault, _claimId)
{
    if (resolutionChallengedAt[_vault][_claimId] == 0) {
        revert CannotDismissUnchallengedResolution();
    }

    if (msg.sender != court) {
        revert CanOnlyBeCalledByCourt();
    }

    _vault.dismissClaim(_claimId);
+   token.safeTransfer(dismissedResolutionBondsReceiver, totalBondsOnClaim[_vault][_claimId]);   

    emit ResolutionDismissed(_vault, _claimId);
}

[aviggiano]

Hey

Nice find

However I'd like to point out that the proposed mitigation is missing an important step as totalBondsToConfiscate is not being reset to 0 after the executeResolution transfer the funds to the expert committee.

Please find below the revised mitigation:

function executeResolution(
    IHATClaimsManager _vault,
    bytes32 _claimId
)

...

    _vault.approveClaim(
        _claimId,
        resolution.bountyPercentage,
        resolution.beneficiary
    );

+   token.safeTransfer(expertCommittee, totalBondsToConfiscate);
+   totalBondsToConfiscate = 0;
    emit ResolutionExecuted(_vault, _claimId);
}

[bahurum]

Hey, thank you
I had put the reset to zero inside _confiscateDisputers() but that is weird and I prefer you suggestion. In that case, one needs also to reset the value to 0 inside dismissResolution().

function dismissResolution(
    IHATClaimsManager _vault,
    bytes32 _claimId
)
    external
    onlyChallengedActiveClaim(_vault, _claimId)
    onlyResolvedDispute(_vault, _claimId)
{

...

    _vault.dismissClaim(_claimId);
+   token.safeTransfer(dismissedResolutionBondsReceiver, totalBondsOnClaim[_vault][_claimId]);   
+   totalBondsToConfiscate = 0;

    emit ResolutionDismissed(_vault, _claimId);
}

[bahurum]

Now that I look again at the recommendation, I notice that totalBondsToConfiscate could mix up between vaults.
In the recommendation all totalBondsToConfiscate should be replaced by a mapping totalBondsToConfiscate[_vault].
Mapping by _vault should be enough since vaults only treat one _claimId at a time, but totalBondsToConfiscate[_vault][_claimId] can be used as well if preferred.

[jellegerbrandy]

So this is a very interesting discussion. The current implementation was by design, so I would not categorize this as a bug in the code. But you are making a good point about the validity fo the design. We'll discuss further internally

[jellegerbrandy]

So although we see your point, we decided to stick to the design we have now.

We have several reasons

• the system of bonds and repayment is already very complex, and we think it makes sense to keep the economics and incentives int he two escalation steps (expert committee then kleros) separate from each other (even if it may come at the cost of some unfairness.
• In your scenario, it remains unclear whether Ann (or Bob) in the end was really right or not. The Expert committee has ruled that Ann was wrong and Bob was right, but that ruling was dismissed by the court, so that is not information we can base our payout on. But the dismissal of the expert committees claim does not imply that Bob should not be refunded, or that Alcie should.
• consider also that this is meant to be an optimistic process - it decision should (almost) never arrive at the court. So this is very much a limit case (which is not a reason to not fix, but it makes it less urgent)