Sherlock contest - GreyArt's results

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.

Handle

GreyArt

Vulnerability details

Impact

As per the documentation, “After 2 weeks without action on an unlocked position arbs can come in to arbRestake(id), 20% of the underlying USDC amount (principal + yield) is at risk for the owner of the position.”

While shares are redeemed for the arbitrager, the owner’s shares are not reduced in the addressShares mapping. This will cause an incorrect value to be returned when fetching the owner’s tokenBalanceOfAddress().

Issue is rated as low severity because there is no impact to funds.

Proof of Concept

Append the following lines into the Do case of arbRestake() in Sherlock.js.

// to make totalStakeShares non-zero after Carol redeems NFT
await meta(this.sherlock.initialStake(this.amount, 10, this.bob.address));
await timeTraveler.increaseTime((this.t2.time.add(weeks12)).toNumber());
await this.sherlock.connect(this.carol).redeemNFT(1);
console.log(`expect Carol's address to have 0 token balance (fully redeemed)`);
expect(await this.sherlock.tokenBalanceOfAddress(this.carol.address)).to.eq(0);

The initial state is Carol, the only party, minting 1 NFT. After arbRestake() is done, if Carol redeems that NFT, she is expected to have 0 token balance. This is however not the case because of her shares are not decremented in arbRestake(). Note that we mint another NFT prior to Carol’s redeem so that totalStakeShares is non-zero. (tokenBalanceOfAddress() returns zero if totalStakeShares is zero).

Recommended Mitigation Steps

Reduce the nftOwner’s addressShares in arbRestake().

function arbRestake(uint256 _id) ... {
	_arbReward = _redeemShares(_id, arbRewardShares, msg.sender);
	// Burn NFT owner's addressShares
	addressShares[nftOwner] -= arbRewardShares;
	...
}

Handle

GreyArt

Vulnerability details

Impact

If there are funds remaining in an old strategy, there is only 1 way to claim those funds which is through Sherlock.updateYieldStrategy() . It is quite an inconvenience to do this.

Recommended Mitigation Steps

Create an additional function to allow anyone to call withdraw / withdraw all from an old strategy. The withdrawn funds should be sent to SherlockCore e.g.

function oldStrategyWithdrawAll(IStrategyManager _oldStrategy) external override {
    _oldStrategy.withdrawAll();
}