hats-finance / HATs-Arbitration-Contracts-0x79a618f675857b45934ca1c413fd5f409cf89735

MIT License
0 stars 1 forks source link

Depositors can withdraw deposits after an inconclusive arbitration process over bounty amount #38

Open hats-bug-reporter[bot] opened 10 months ago

hats-bug-reporter[bot] commented 10 months ago

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

Description: Description\ A depositor of the HATVault has the possibility to withdraw its deposit and avoid paying for a bounty if the arbitration process over the bounty amount is not conclusive (the claim is disputed, the dispute is accepted by the expert committee but the resolution is dismissed by the court).

Attack Scenario

  1. A claim is submitted by the committee to pay a bounty as medium severity
  2. The Whitehat disputes over the severity and argues for a high severity
  3. The expert committee accepts the dispute
  4. The project (or someone else) challenges the expert committee's resolution
  5. The court does not agree with the expert committee and dismisses the resolution by calling dismissResolution(). The claim is dismissed and no payout is made

Some depositors in the vault know that the bug is real and some payout must be made either for medium or high severity. They Call HATVault.withdrawRequest() in the period of time between 1. and 5. Even if they don't know beforehand if the arbitration process will end in a resolution dismissal (as above) or how much time it will take, it is likely that some depositor will manage to time the withdraw request right so that the resolution is dismissed during their withdrawRequestEnablePeriod, and they are able to withdraw their deposit in full just after the resolution is dismissed.

When a new claim is made for the bounty and the claim is approved after a second arbitration, the bounty that will be payed will be smaller because some depositors exited the vault.

Recommendation\ A solution to this issue is to block calls to withdrawRequest() during an active claim and to check that withdrawRequestEnablePeriod + withdrawRequestPendingPeriod time has elapsed since the creation of the claim so that any withdraw request created before the claim submission has expired.

function withdrawRequest() external nonReentrant {
    // set the withdrawEnableStartTime time to be withdrawRequestPendingPeriod from now
    // solhint-disable-next-line not-rely-on-time
+   if (withdrawPaused) revert CannotRequestWithdrawDuringActiveClaim();
    uint256 _withdrawEnableStartTime = block.timestamp + registry.getWithdrawRequestPendingPeriod();
    address msgSender = _msgSender();
    withdrawEnableStartTime[msgSender] = _withdrawEnableStartTime;
    emit WithdrawRequest(msgSender, _withdrawEnableStartTime);
}
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();
    }

+   IHATClaimsManager.Claim memory claim = _vault.getActiveClaim();
+   if (
+       block.timestamp < claim.createdAt +
+       registry.getWithdrawRequestEnablePeriod() +
+       registry.getWithdrawRequestPendingPeriod()
+   )  {
+        revert CannotDismissYet();
+   }
+
    _vault.dismissClaim(_claimId);

    emit ResolutionDismissed(_vault, _claimId);
}
jellegerbrandy commented 10 months ago

This is by design.

Our idea here is that the whole thing is an optimistic process - i.e. we assume all parties are in good faith, and if they are not this is an exception, not the rule. So in the scenerio you describe, the vault committee's claim has been overruled by the expert comittee's claim, and the expert committee's claim is then dismissed by the court. If things are working as they should, this should not happen (i..e we normally expect the vault committee's claim to be fair, and if it is not, then definitely the expert committee's claim should be fair). If both of these committee's fail (according to the kleros court), something has Really Gone Wrong.

We think it is good that in this case, depositors can withdraw from the vault.