SherlockClaimManager: Incorrect amounts needed and paid for escalated claims

[code423n4]

Handle

GreyArt

Vulnerability details

Impact

When escalating claims, the documentation states that the protocol agent is required to pay and stake a certain amount for the process. If the covered protocol is proven correct, then the amount specified by the claim will be paid out. They will also receive the stake amount back in full. If the covered protocol's escalation is not successful, then the amount specified by the claim is not paid out and the stake amount is not returned.

The protocol agent is reasonably expected to pay the following:

• The stake (BOND) and
• UMA’s final fee

In reality, the protocol agent will end up paying more, as we shall see in the proof of concept.

Proof of Concept

Let us assume the following:

• BOND = 9600 as defined in SherlockClaimManager
• umaFee = 400 (at the time of writing, this value has been updated to 1500 USDC: see [Store.computeFinalFee(usdc)](https://etherscan.io/address/0x54f44eA3D2e7aA0ac089c4d8F7C93C27844057BF#readContract)).

On invoking escalate(), the following amounts are required:

1. BOND + umaFee = 9600 + 400 will be transferred to UMA when invoking requestAndProposePriceFor()

// Taken from https://github.com/UMAprotocol/protocol/blob/master/packages/core/contracts/oracle/implementation/SkinnyOptimisticOracle.sol
// Only relevant lines are referenced
uint256 finalFee = _getStore().computeFinalFee(address(currency)).rawValue;
request.finalFee = finalFee;
totalBond = request.bond.add(request.finalFee);
if (totalBond > 0) currency.safeTransferFrom(msg.sender, address(this), totalBond);

1. Another BOND + umaFee = 9600 + 400 will be transfered when invoking disputePriceFor()

// Taken from https://github.com/UMAprotocol/protocol/blob/master/packages/core/contracts/oracle/implementation/SkinnyOptimisticOracle.sol#L389-L390
totalBond = request.bond.add(request.finalFee);
if (totalBond > 0) request.currency.safeTransferFrom(msg.sender, address(this), totalBond);

However, what’s important to note is that UMA will “burn” half of the BOND collected + final fee. This will go against the claim that the protocol agent will be able to reclaim his stake in full.

StoreInterface store = _getStore();

// Avoids stack too deep compilation error.
{
    // Along with the final fee, "burn" part of the loser's bond to ensure that a larger bond always makes it
    // proportionally more expensive to delay the resolution even if the proposer and disputer are the same
    // party.
    uint256 burnedBond = _computeBurnedBond(disputedRequest);

    // The total fee is the burned bond and the final fee added together.
    uint256 totalFee = request.finalFee.add(burnedBond);

    if (totalFee > 0) {
        request.currency.safeIncreaseAllowance(address(store), totalFee);
        _getStore().payOracleFeesErc20(address(request.currency), FixedPoint.Unsigned(totalFee));
    }
}

function _computeBurnedBond(Request memory request) private pure returns (uint256) {
  // burnedBond = floor(bond / 2)
  return request.bond.div(2);
}

We finally note that on settlement, the eventual payout is

// Winner gets:
// - Their bond back.
// - The unburned portion of the loser's bond: proposal bond (not including final fee) - burned bond.
// - Their final fee back.
// - The request reward (if not already refunded -- if refunded, it will be set to 0).
payout = request.bond.add(request.bond.sub(_computeBurnedBond(settledRequest))).add(request.finalFee).add(
    request.reward
);
request.currency.safeTransfer(disputeSuccess ? request.disputer : request.proposer, payout);

Hence, in reality, the protocol agent will only receive 9600 * 2 - 4800 + 400 = 14800 should the dispute be successful. We note that the burnt amount of 4800 / 2 + 400 = 5200 has been taken by UMA.

One can further verify this behaviour by looking at a past resolution of another protocol:

https://dashboard.tenderly.co/tx/main/0x0f03f73a2093e385146791e8f2739dbc04b39145476d6940776680243460100f/debugger?trace=0.6.1

The above example has a bond is 0.0075 ETH, with UMA’s final fee being 0.2 ETH. We see that UMA takes 0.2 + 0.5 * 0.0075 = 0.02375 ETH.

Thus, we see that the protocol agent will be charged disproportionally to what is expected.

Recommended Mitigation Steps

We suggest changing the parameters of requestAndProposePriceFor() to

UMA.requestAndProposePriceFor(
  UMA_IDENTIFIER, // Sherlock ID so UMA knows the request came from Sherlock
  claim.timestamp, // Timestamp to identify the request
  claim.ancillaryData, // Ancillary data such as the coverage agreement
  TOKEN, // USDC
  BOND, // While sherlock handles rewards on its own, we use the BOND as the reward
  // because using it as UMA's bond would result in 0.5 * BOND charged by UMA excluding final fee
  1, // Ideally 0, but 0 = final fee used. Hence, we set it to the next lowest 
  // possible value
  LIVENESS, // Proposal liveness
  address(sherlockCore), // Sherlock core address
  0 // price
);

where BOND becomes the reward and the actual bond for UMA is 1. Ideally, it should be set to 0, but if set as such, UMA interprets it to use the final fee as the bond amount instead.

[request.bond = bond != 0 ? bond : finalFee;](https://github.com/UMAprotocol/protocol/blob/master/packages/core/contracts/oracle/implementation/SkinnyOptimisticOracle.sol#L321)

This way, the actual amount required from the protocol agent is the BOND + 2 * (USDC wei + umaFee) for the process. He will additionally be returned his BOND + umaFee if his dispute is successful.

[Evert0x]

Non critical as documentation is incorrect about this

[jack-the-pug]

Downgrading to Med as it's mostly because the documentation is incorrect.

[rcstanciu]

@jack-the-pug The docs have been updated to explain the correct burned amount.

[jack-the-pug]

@jack-the-pug The docs have been updated to explain the correct burned amount.

Thank you @rcstanciu

While I agreed that the issue only impacts a small sum of users and the impact is not significant. And the root cause of this issue may not be a wrong implementation but actual wrong documentation.

However, I still tend to make this a Med rather than a Low for the following reasons:

1. A wrong documentation is arguably indistinguishable from a wrong implementation that actually violates the intention of the design, especial from an outsider's pov;
2. This write-up indicates a dedicated and in-depth effort of the warden by digging into the documentation and trying to understand the intention and cross-compare with the actual implementation to find any differences. As a judge, part of my job is to make sure that the wardens' findings are being rewarded in a just and fair manner.

Therefore, I'm keeping this as a Med and I encourage the future wardens to continue finding the inconsistency between the documentation and the implementation.

Keep up the good work! GreyArt is a great name btw.