Resolution cannot completely cancel the payment of a bounty

[hats-bug-reporter[bot]]

Github username: @bahurum Submission hash (on-chain): 0xdafc8513cfc78fed84be397f4852fac8346c8cda9c7c472e16659b2f42f61cbf Severity: low

Description: Description\ The expert committee cannot take a resolution to cancel a claim for a bounty payment completely. Since _bountyPercentage = 0 is reserved for preserving the original claim's bounty amount, the resolution will always require the payment of a bounty, even if very small.

Attack Scenario

1. The committee submits a claim
2. One of the depositors in the vault disputes the claim and argues that the submission is invalid and no bounty should be paid at all
3. The expert committee agrees that no bounty should be paid and calls acceptDispute(). In choosing the parameters for the call, _bountyPercentage = 0 is reserved to the case where the expert committee confirms the committee's bounty amount (see HATClaimsManager.sol#L234-L235). The best that the expert committee can do is to set a very low amount for _bountyPercentage.
4. The resolution is challenged and the court rules in favor of the expert committee(). approveClaim() is called and a very small payout is made (including the creation of a token lock).

How the process ends at 4. is not ideal, and ideally at 3. the expert committee should be able to specify that it doesn't want a payout, and then in approveClaim() no payout should made.

Recommendation\ Consider using a special value of the _bountyPercentage param of acceptDispute() as reserved for canceled bounties. The value 1 could be used (corresponds to 0.01% of the vault so will never be used otherwise).

In approveClaim() do not make the payout if the _bountyPercentage is equal to the special value:

function approveClaim(bytes32 _claimId, uint16 _bountyPercentage, address _beneficiary) external nonReentrant isActiveClaim(_claimId) {
    Claim memory _claim = activeClaim;
    delete activeClaim;

    // solhint-disable-next-line not-rely-on-time
    if (block.timestamp >= _claim.createdAt + _claim.challengePeriod + _claim.challengeTimeOutPeriod) {
        // cannot approve an expired claim
        revert ClaimExpired();
    } 
    if (_claim.challengedAt != 0) {
        // the claim was challenged, and only the arbitrator can approve it, within the timeout period
        if (
            msg.sender != _claim.arbitrator ||
            // solhint-disable-next-line not-rely-on-time
            block.timestamp >= _claim.challengedAt + _claim.challengeTimeOutPeriod
        )
            revert ChallengedClaimCanOnlyBeApprovedByArbitratorUntilChallengeTimeoutPeriod();
        // the arbitrator can update the bounty if needed
        if (_claim.arbitratorCanChangeBounty && _bountyPercentage != 0) {
            _claim.bountyPercentage = _bountyPercentage;
        }

        if (_claim.arbitratorCanChangeBeneficiary && _beneficiary != address(0)) {
            _claim.beneficiary = _beneficiary;
        }
    } else {
        // the claim can be approved by anyone if the challengePeriod passed without a challenge
        if (
            // solhint-disable-next-line not-rely-on-time
            block.timestamp <= _claim.createdAt + _claim.challengePeriod
        ) 
            revert UnchallengedClaimCanOnlyBeApprovedAfterChallengePeriod();
    }

    vault.setWithdrawPaused(false);

+   if (_claim.bountyPercentage != 1) {
+
    if (_claim.bountyPercentage == HUNDRED_PERCENT) {
        vault.destroyVault();
    }

    address tokenLock;

    IHATClaimsManager.ClaimBounty memory claimBounty = _calcClaimBounty(
        _claim.bountyPercentage,
        _claim.bountyGovernanceHAT,
        _claim.bountyHackerHATVested
    );

    vault.makePayout(
        claimBounty.committee +
        claimBounty.governanceHat +
        claimBounty.hacker +
        claimBounty.hackerHatVested +
        claimBounty.hackerVested
    );

    IERC20 _asset = IERC20(vault.asset());
    if (claimBounty.hackerVested > 0) {
        //hacker gets part of bounty to a vesting contract
        tokenLock = tokenLockFactory.createTokenLock(
            address(_asset),
            isTokenLockRevocable ? committee : 0x0000000000000000000000000000000000000000, // owner
            _claim.beneficiary,
            claimBounty.hackerVested,
            // solhint-disable-next-line not-rely-on-time
            block.timestamp, //start
            // solhint-disable-next-line not-rely-on-time
            block.timestamp + vestingDuration, //end
            vestingPeriods,
            0, //no release start
            0, //no cliff
            isTokenLockRevocable,
            false
        );
        _asset.safeTransfer(tokenLock, claimBounty.hackerVested);
    }

    _asset.safeTransfer(_claim.beneficiary, claimBounty.hacker);
    _asset.safeTransfer(_claim.committee, claimBounty.committee);

    // send to the registry the amount of tokens which should be swapped 
    // to HAT so it could call swapAndSend in a separate tx.
    IHATVaultsRegistry _registry = registry;
    _asset.safeApprove(address(_registry), claimBounty.hackerHatVested + claimBounty.governanceHat);
    _registry.addTokensToSwap(
        _asset,
        _claim.beneficiary,
        claimBounty.hackerHatVested,
        claimBounty.governanceHat
    );

    // make sure to reset approval
    _asset.safeApprove(address(_registry), 0);

+   }
+
    emit ApproveClaim(
        _claimId,
        _claim.committee,
        msg.sender,
        _claim.beneficiary,
        _claim.bountyPercentage,
        tokenLock,
        claimBounty
    );
}

[jellegerbrandy]

We have dismissClaim for that though

[bahurum]

So in this case the expert committee would do nothing and just let the challenge period pass. Wouldn't it be better if the expert committee has an option to end the arbitration immediatly?

Also, I had assumed that if the expert committee does not agree with the committee, then the expert committee decision (resolution) should be challengeable. In other words, I assumed that the expert committee cannot unilaterally give a definitive ruling against the committee.