test: improve Oracle test coverage

defi-wonderland · Dec 4, 2023 · a0ca2b8 · a0ca2b8
1 parent 2248f92
commit a0ca2b8
Show file tree

Hide file tree

Showing 4 changed files with 318 additions and 142 deletions.
diff --git a/solidity/contracts/Oracle.sol b/solidity/contracts/Oracle.sol
@@ -130,16 +130,15 @@ contract Oracle is IOracle {
   ) external returns (bytes32 _disputeId) {
     _disputeId = _validateDispute(_request, _response, _dispute);
 
-    // TODO: Check for createdAt instead?
-    // if(_participants[_requestId].length == 0) {
-    //   revert();
-    // }
+    if (createdAt[_dispute.requestId] == 0) {
+      revert Oracle_InvalidDisputeBody();
+    }
 
     if (finalizedAt[_response.requestId] != 0) {
       revert Oracle_AlreadyFinalized(_response.requestId);
     }
 
-    // TODO: Allow multiple disputes per response to prevent an attacker starting and losing a dispute,
+    // TODO: Allow multiple disputes per response to prevent an attacker from starting and losing a dispute,
     // making it impossible for non-malicious actors to dispute a response?
     if (disputeOf[_dispute.responseId] != bytes32(0)) {
       revert Oracle_ResponseAlreadyDisputed(_dispute.responseId);
@@ -311,42 +310,9 @@ contract Oracle is IOracle {
 
     // Finalizing without a response (by passing a Response with `requestId` == 0x0)
     if (_response.requestId == bytes32(0)) {
-      _requestId = keccak256(abi.encode(_request));
-      bytes32[] memory _responses = getResponseIds(_requestId);
-      uint256 _responsesAmount = _responses.length;
-
-      if (_responsesAmount != 0) {
-        for (uint256 _i = 0; _i < _responsesAmount;) {
-          _responseId = _responses[_i];
-          bytes32 _disputeId = disputeOf[_responseId];
-          DisputeStatus _status = disputeStatus[_disputeId];
-
-          if (_status != DisputeStatus.None && _status != DisputeStatus.Lost) {
-            revert Oracle_InvalidFinalizedResponse(_responseId);
-          }
-
-          unchecked {
-            ++_i;
-          }
-        }
-
-        // Reset the variable to emit bytes32(0) in the event
-        _responseId = bytes32(0);
-      }
+      _requestId = _finalizeWithoutResponse(_request);
     } else {
-      _responseId = _validateResponse(_request, _response);
-      _requestId = _response.requestId;
-      if (!_matchBytes32(_responseId, _responseIds[_requestId])) {
-        revert Oracle_InvalidFinalizedResponse(_responseId);
-      }
-
-      DisputeStatus _status = disputeStatus[disputeOf[_responseId]];
-
-      if (_status != DisputeStatus.None && _status != DisputeStatus.Lost) {
-        revert Oracle_InvalidFinalizedResponse(_responseId);
-      }
-
-      _finalizedResponses[_requestId] = _responseId;
+      (_requestId, _responseId) = _finalizeWithResponse(_request, _response);
     }
 
     if (finalizedAt[_requestId] != 0) {
@@ -370,6 +336,62 @@ contract Oracle is IOracle {
     emit OracleRequestFinalized(_requestId, _responseId, msg.sender, block.number);
   }
 
+  /**
+   * @notice Finalizing a request that either does not have any responses or only has disputed responses
+   *
+   * @param _request The request to be finalized
+   * @return _requestId The id of the finalized request
+   */
+  function _finalizeWithoutResponse(IOracle.Request calldata _request) internal view returns (bytes32 _requestId) {
+    _requestId = keccak256(abi.encode(_request));
+    bytes32[] memory _responses = getResponseIds(_requestId);
+    uint256 _responsesAmount = _responses.length;
+
+    if (_responsesAmount != 0) {
+      for (uint256 _i = 0; _i < _responsesAmount;) {
+        bytes32 _responseId = _responses[_i];
+        bytes32 _disputeId = disputeOf[_responseId];
+        DisputeStatus _status = disputeStatus[_disputeId];
+
+        // If there is an undisputed response or with a lost dispute, must finalize with it
+        if (_status == DisputeStatus.None || _status == DisputeStatus.Lost) {
+          revert Oracle_FinalizableResponseExists(_responseId);
+        }
+
+        unchecked {
+          ++_i;
+        }
+      }
+    }
+  }
+
+  /**
+   * @notice Finalizing a request with a response
+   *
+   * @param _request The request to be finalized
+   * @param _response The final response
+   * @return _requestId The id of the finalized request
+   * @return _responseId The id of the final response
+   */
+  function _finalizeWithResponse(
+    IOracle.Request calldata _request,
+    IOracle.Response calldata _response
+  ) internal returns (bytes32 _requestId, bytes32 _responseId) {
+    _responseId = _validateResponse(_request, _response);
+    _requestId = _response.requestId;
+    if (!_matchBytes32(_responseId, _responseIds[_requestId])) {
+      revert Oracle_InvalidFinalizedResponse();
+    }
+
+    DisputeStatus _status = disputeStatus[disputeOf[_responseId]];
+
+    if (_status != DisputeStatus.None && _status != DisputeStatus.Lost) {
+      revert Oracle_InvalidFinalizedResponse();
+    }
+
+    _finalizedResponses[_requestId] = _responseId;
+  }
+
   /**
    * @notice Stores a request in the contract and configures it in the modules
    *
@@ -380,7 +402,6 @@ contract Oracle is IOracle {
   function _createRequest(Request calldata _request, bytes32 _ipfsHash) internal returns (bytes32 _requestId) {
     uint256 _requestNonce = totalRequestCount++;
 
-    // @audit what about removing nonces? or how we avoid nonce clashing?
     if (_requestNonce != _request.nonce || msg.sender != _request.requester) revert Oracle_InvalidRequestBody();
 
     _requestId = keccak256(abi.encode(_request));

diff --git a/solidity/interfaces/IOracle.sol b/solidity/interfaces/IOracle.sol
@@ -103,9 +103,14 @@ interface IOracle {
 
   /**
    * @notice Thrown when trying to finalize a request with an invalid response
-   * @param _responseId The id of the response
    */
-  error Oracle_InvalidFinalizedResponse(bytes32 _responseId);
+  error Oracle_InvalidFinalizedResponse();
+
+  /**
+   * @notice Thrown when trying to finalize a request without a response while there is, in fact, a response
+   * @param _responseId The id of the response that would be suitable for finalization
+   */
+  error Oracle_FinalizableResponseExists(bytes32 _responseId);
 
   /**
    * @notice Thrown when trying to resolve or escalate an invalid dispute
@@ -148,12 +153,12 @@ interface IOracle {
    * @notice All available statuses a dispute can have
    */
   enum DisputeStatus {
-    None,
-    Active,
-    Escalated,
-    Won,
-    Lost,
-    NoResolution
+    None, // The dispute has not been started yet
+    Active, // The dispute is active and can be escalated or resolved
+    Escalated, // The dispute is being resolved by the resolution module
+    Won, // The disputer has won the dispute
+    Lost, // The disputer has lost the dispute
+    NoResolution // The dispute was inconclusive
   }
 
   /*///////////////////////////////////////////////////////////////

diff --git a/solidity/test/integration/Finalization.t.sol b/solidity/test/integration/Finalization.t.sol
@@ -73,7 +73,7 @@ contract Integration_Finalization is IntegrationBase {
   }
 
   /**
-   * @notice Test to check that finalizing a request with a ongoing dispute with revert.
+   * @notice Test to check that finalizing a request with a ongoing dispute will revert.
    */
   function test_revertFinalizeWithDisputedResponse() public {
     mockRequest.finalityModuleData =
@@ -87,18 +87,18 @@ contract Integration_Finalization is IntegrationBase {
     oracle.createRequest(mockRequest, _ipfsHash);
 
     vm.prank(proposer);
-    bytes32 _responseId = oracle.proposeResponse(mockRequest, mockResponse);
+    oracle.proposeResponse(mockRequest, mockResponse);
 
     vm.prank(disputer);
     oracle.disputeResponse(mockRequest, mockResponse, mockDispute);
 
     vm.prank(_finalizer);
-    vm.expectRevert(abi.encodeWithSelector(IOracle.Oracle_InvalidFinalizedResponse.selector, _responseId));
+    vm.expectRevert(IOracle.Oracle_InvalidFinalizedResponse.selector);
     oracle.finalize(mockRequest, mockResponse);
   }
 
   /**
-   * @notice Test to check that finalizing a request with a ongoing dispute with revert.
+   * @notice Test to check that finalizing a request with a ongoing dispute will revert.
    */
   function test_revertFinalizeInDisputeWindow() public {
     mockRequest.finalityModuleData =