Sherlock contest - sirhashalot's results

Handle

sirhashalot

Vulnerability details

Impact

The OpenZeppelin SafeERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code. Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219.

Proof of Concept

The deprecated function is found in:

• managers/AaveV2Strategy.sol line 71
• managers/SherlockClaimManager.sol line 422 and line 465

Recommended Mitigation Steps

As suggested by the OpenZeppelin comment, replace safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance().

• For line 422 of SherlockClaimManager.sol, use TOKEN.safeIncreaseAllowance(address(UMA), _amount)
• For line 465 of SherlockClaimManager.sol, use TOKEN.safeDecreaseAllowance(address(UMA), 0)
• For line 71 of AaveV2Strategy.sol, use want.safeIncreaseAllowance(address(lp), type(uint256).max)

Handle

sirhashalot

Vulnerability details

Impact

The _isCleanupState() function is only called once in SherlockClaimManager.sol. This function is short and it can save gas to embed the function's logic inline.

Proof of Concept

Current function's code

  function _isCleanupState(State _oldState) internal pure returns (bool) {
    if (_oldState == State.SpccDenied) return true;
    if (_oldState == State.SpccPending) return true;
    return false;
  }
  
  if (_isCleanupState(_oldState) == false) revert   InvalidState();

Proposed alternative removing the _isCleanupState() function with inline logic.

  if (_oldState != State.SpccDenied && _oldState != State.SpccPending) revert   InvalidState();

Recommended Mitigation Steps

See code above. Replace the _isCleanupState() function call with the function's logic if gas savings is a priority.

Handle

sirhashalot

Vulnerability details

Impact

The Sherlock protocol allows agents to make a claim against previous higher coverage amounts if the coverage amount has been lowered. This feature fails to work as expected if the agent makes more than one call to protocolUpdate() lowering the coverage amount.

Proof of Concept

The SherlockProtocolManager.sol contract has the following comments describing the previousCoverage mapping:

// Previous amount of coverage for a protocol
// Previous is also tracked in case a protocol lowers their coverage amount but still needs to make a claim on the old, higher amount
mapping(bytes32 => uint256) internal previousCoverage;

This shows the protocol needs to track historic coverage values because an agent can still make claims against the previous values. But the protocol is currently only able to track one previous coverage value. This means that if an agent calls the protocolUpdate() function twice, lowering their coverage value both times, they would be unable to make a claim for their original higher coverage amount.

The previousCoverage value gets set in the protocolUpdate function. It is later returned in the coverageAmounts function which is used in the startClaim function. The startClaim function is unable to retrieve coverage amounts beyond the current and previous values, even though the protocol seeking a claim may have been covered under a higher coverage amount at the time the agent is requesting the claim for.

Recommended Mitigation Steps

The most accurate solution would be to use a dynamic array to track all previous coverage values, which would require additional code to loop through the previous coverage values to find the maximum. An inelegant hack is that any time the protocolUpdate() function is called by the owner (with onlyOwner modifier privilege) and the previousCoverage value that will be erased should remain in the previousCoverage, a temporary intermediate call to protocolUpdate() can be done to assign the higher previousCoverage value to the currentCoverage value temporarily. Then the actual currentCoverage value is assigned and the old previousCoverage value can be pushed back to the prevousCoverage variable. This inelegant hack would not require a code change, but would rely on off-chain processes to work properly and therefore should be clearly documented if used.