From f8f22ebe76c1a9f08ebdd4c86fe88eedcc9bbb2f Mon Sep 17 00:00:00 2001 From: 0xJabberwock <0xjabberwock@defi.sucks> Date: Mon, 5 Aug 2024 16:29:28 -0300 Subject: [PATCH] fix: low-level calls (#56) * fix: low-level calls * feat: separate callback interface for verifier --- .../modules/dispute/CircuitResolverModule.sol | 20 ++++---- .../modules/finality/CallbackModule.sol | 3 +- .../finality/MultipleCallbacksModule.sol | 3 +- solidity/interfaces/IProphetCallback.sol | 11 ++++ solidity/interfaces/IProphetVerifier.sol | 11 ++++ solidity/test/integration/Finalization.t.sol | 50 +++---------------- solidity/test/integration/IntegrationBase.sol | 2 + solidity/test/mocks/MockCallback.sol | 9 ++-- .../dispute/CircuitResolverModule.t.sol | 34 ++++++++----- .../modules/finality/CallbackModule.t.sol | 7 ++- .../finality/MultipleCallbacksModule.t.sol | 6 ++- 11 files changed, 82 insertions(+), 74 deletions(-) create mode 100644 solidity/interfaces/IProphetCallback.sol create mode 100644 solidity/interfaces/IProphetVerifier.sol diff --git a/solidity/contracts/modules/dispute/CircuitResolverModule.sol b/solidity/contracts/modules/dispute/CircuitResolverModule.sol index 5a7b368b..a0d04782 100644 --- a/solidity/contracts/modules/dispute/CircuitResolverModule.sol +++ b/solidity/contracts/modules/dispute/CircuitResolverModule.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.19; import {IModule, Module} from '@defi-wonderland/prophet-core-contracts/solidity/contracts/Module.sol'; import {IOracle} from '@defi-wonderland/prophet-core-contracts/solidity/interfaces/IOracle.sol'; +import {IProphetVerifier} from '../../../interfaces/IProphetVerifier.sol'; import {ICircuitResolverModule} from '../../../interfaces/modules/dispute/ICircuitResolverModule.sol'; contract CircuitResolverModule is Module, ICircuitResolverModule { @@ -68,17 +69,18 @@ contract CircuitResolverModule is Module, ICircuitResolverModule { IOracle.Dispute calldata _dispute ) external onlyOracle { RequestParameters memory _params = decodeRequestData(_request.disputeModuleData); + IOracle.DisputeStatus _status; - (bool _success, bytes memory _correctResponse) = _params.verifier.call(_params.callData); + try IProphetVerifier(_params.verifier).prophetVerify(_params.callData) returns (bytes memory _correctResponse) { + _correctResponses[_response.requestId] = _correctResponse; - if (!_success) revert CircuitResolverModule_VerificationFailed(); - - _correctResponses[_response.requestId] = _correctResponse; - - IOracle.DisputeStatus _status = _response.response.length != _correctResponse.length - || keccak256(_response.response) != keccak256(_correctResponse) - ? IOracle.DisputeStatus.Won - : IOracle.DisputeStatus.Lost; + _status = _response.response.length != _correctResponse.length + || keccak256(_response.response) != keccak256(_correctResponse) + ? IOracle.DisputeStatus.Won + : IOracle.DisputeStatus.Lost; + } catch { + revert CircuitResolverModule_VerificationFailed(); + } emit ResponseDisputed({ _requestId: _response.requestId, diff --git a/solidity/contracts/modules/finality/CallbackModule.sol b/solidity/contracts/modules/finality/CallbackModule.sol index 06755abd..968b0dde 100644 --- a/solidity/contracts/modules/finality/CallbackModule.sol +++ b/solidity/contracts/modules/finality/CallbackModule.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.19; import {IModule, Module} from '@defi-wonderland/prophet-core-contracts/solidity/contracts/Module.sol'; import {IOracle} from '@defi-wonderland/prophet-core-contracts/solidity/interfaces/IOracle.sol'; +import {IProphetCallback} from '../../../interfaces/IProphetCallback.sol'; import {ICallbackModule} from '../../../interfaces/modules/finality/ICallbackModule.sol'; contract CallbackModule is Module, ICallbackModule { @@ -28,7 +29,7 @@ contract CallbackModule is Module, ICallbackModule { ) external override(Module, ICallbackModule) onlyOracle { RequestParameters memory _params = decodeRequestData(_request.finalityModuleData); - _params.target.call(_params.data); + IProphetCallback(_params.target).prophetCallback(_params.data); emit Callback(_response.requestId, _params.target, _params.data); emit RequestFinalized(_response.requestId, _response, _finalizer); } diff --git a/solidity/contracts/modules/finality/MultipleCallbacksModule.sol b/solidity/contracts/modules/finality/MultipleCallbacksModule.sol index d9775b25..32c4d845 100644 --- a/solidity/contracts/modules/finality/MultipleCallbacksModule.sol +++ b/solidity/contracts/modules/finality/MultipleCallbacksModule.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.19; import {IModule, Module} from '@defi-wonderland/prophet-core-contracts/solidity/contracts/Module.sol'; import {IOracle} from '@defi-wonderland/prophet-core-contracts/solidity/interfaces/IOracle.sol'; +import {IProphetCallback} from '../../../interfaces/IProphetCallback.sol'; import {IMultipleCallbacksModule} from '../../../interfaces/modules/finality/IMultipleCallbacksModule.sol'; contract MultipleCallbacksModule is Module, IMultipleCallbacksModule { @@ -30,7 +31,7 @@ contract MultipleCallbacksModule is Module, IMultipleCallbacksModule { uint256 _length = _params.targets.length; for (uint256 _i; _i < _length;) { - _params.targets[_i].call(_params.data[_i]); + IProphetCallback(_params.targets[_i]).prophetCallback(_params.data[_i]); emit Callback(_response.requestId, _params.targets[_i], _params.data[_i]); unchecked { ++_i; diff --git a/solidity/interfaces/IProphetCallback.sol b/solidity/interfaces/IProphetCallback.sol new file mode 100644 index 00000000..24261920 --- /dev/null +++ b/solidity/interfaces/IProphetCallback.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.19; + +interface IProphetCallback { + /** + * @notice Callback function for the Prophet modules + * @param _callData The encoded data for the callback + * @return _callResponse The encoded response for the callback + */ + function prophetCallback(bytes calldata _callData) external returns (bytes memory _callResponse); +} diff --git a/solidity/interfaces/IProphetVerifier.sol b/solidity/interfaces/IProphetVerifier.sol new file mode 100644 index 00000000..56c20ac8 --- /dev/null +++ b/solidity/interfaces/IProphetVerifier.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.19; + +interface IProphetVerifier { + /** + * @notice Verification function for the Prophet modules + * @param _callData The encoded data for the verification + * @return _callResponse The encoded response for the verification + */ + function prophetVerify(bytes calldata _callData) external returns (bytes memory _callResponse); +} diff --git a/solidity/test/integration/Finalization.t.sol b/solidity/test/integration/Finalization.t.sol index 76ec96d4..23a988ef 100644 --- a/solidity/test/integration/Finalization.t.sol +++ b/solidity/test/integration/Finalization.t.sol @@ -5,58 +5,22 @@ import './IntegrationBase.sol'; contract Integration_Finalization is IntegrationBase { address internal _finalizer = makeAddr('finalizer'); - address internal _callbackTarget = makeAddr('target'); function setUp() public override { super.setUp(); - vm.etch(_callbackTarget, hex'069420'); - - _setFinalizationModule( - address(_callbackModule), - abi.encode(ICallbackModule.RequestParameters({target: _callbackTarget, data: bytes('')})) - ); - _deposit(_accountingExtension, requester, usdc, _expectedReward); _deposit(_accountingExtension, proposer, usdc, _expectedBondSize); _deposit(_accountingExtension, disputer, usdc, _expectedBondSize); } - /** - * @notice Test to check if another module can be set as callback module. - */ - function test_targetIsAnotherModule() public { - _setFinalizationModule( - address(_callbackModule), - abi.encode( - ICallbackModule.RequestParameters({ - target: address(_callbackModule), - data: abi.encodeWithSignature('callback()') - }) - ) - ); - - _createRequest(); - _proposeResponse(); - - // Traveling to the end of the dispute window - vm.roll(_expectedDeadline + 1 + _baseDisputeWindow); - - vm.prank(_finalizer); - oracle.finalize(mockRequest, mockResponse); - - // Check: is response finalized? - bytes32 _finalizedResponseId = oracle.finalizedResponseId(_getId(mockRequest)); - assertEq(_finalizedResponseId, _getId(mockResponse)); - } - /** * @notice Finalization data is set and callback calls are made. */ function test_makeAndIgnoreLowLevelCalls(bytes memory _calldata) public { _setFinalizationModule( address(_callbackModule), - abi.encode(ICallbackModule.RequestParameters({target: _callbackTarget, data: _calldata})) + abi.encode(ICallbackModule.RequestParameters({target: address(_mockCallback), data: _calldata})) ); _createRequest(); @@ -65,8 +29,8 @@ contract Integration_Finalization is IntegrationBase { // Traveling to the end of the dispute window vm.roll(_expectedDeadline + 1 + _baseDisputeWindow); - // Check: all low-level calls are made? - vm.expectCall(_callbackTarget, _calldata); + // Check: all external calls are made? + vm.expectCall(address(_mockCallback), abi.encodeWithSelector(IProphetCallback.prophetCallback.selector, _calldata)); vm.prank(_finalizer); oracle.finalize(mockRequest, mockResponse); @@ -126,7 +90,7 @@ contract Integration_Finalization is IntegrationBase { function test_finalizeWithUndisputedResponse(bytes calldata _calldata) public { _setFinalizationModule( address(_callbackModule), - abi.encode(ICallbackModule.RequestParameters({target: _callbackTarget, data: _calldata})) + abi.encode(ICallbackModule.RequestParameters({target: address(_mockCallback), data: _calldata})) ); _createRequest(); @@ -135,7 +99,7 @@ contract Integration_Finalization is IntegrationBase { // Traveling to the end of the dispute window vm.roll(_expectedDeadline + 1 + _baseDisputeWindow); - vm.expectCall(_callbackTarget, _calldata); + vm.expectCall(address(_mockCallback), abi.encodeWithSelector(IProphetCallback.prophetCallback.selector, _calldata)); vm.prank(_finalizer); oracle.finalize(mockRequest, mockResponse); @@ -150,10 +114,10 @@ contract Integration_Finalization is IntegrationBase { function test_revertFinalizeBeforeDeadline(bytes calldata _calldata) public { _setFinalizationModule( address(_callbackModule), - abi.encode(ICallbackModule.RequestParameters({target: _callbackTarget, data: _calldata})) + abi.encode(ICallbackModule.RequestParameters({target: address(_mockCallback), data: _calldata})) ); - vm.expectCall(_callbackTarget, _calldata); + vm.expectCall(address(_mockCallback), abi.encodeWithSelector(IProphetCallback.prophetCallback.selector, _calldata)); _createRequest(); _proposeResponse(); diff --git a/solidity/test/integration/IntegrationBase.sol b/solidity/test/integration/IntegrationBase.sol index f5ce3e33..a6f3de5b 100644 --- a/solidity/test/integration/IntegrationBase.sol +++ b/solidity/test/integration/IntegrationBase.sol @@ -34,6 +34,8 @@ import {HttpRequestModule, IHttpRequestModule} from '../../contracts/modules/req import {ArbitratorModule, IArbitratorModule} from '../../contracts/modules/resolution/ArbitratorModule.sol'; import {BondedResponseModule, IBondedResponseModule} from '../../contracts/modules/response/BondedResponseModule.sol'; +import {IProphetCallback} from '../../interfaces/IProphetCallback.sol'; + import {MockArbitrator} from '../mocks/MockArbitrator.sol'; import {MockCallback} from '../mocks/MockCallback.sol'; diff --git a/solidity/test/mocks/MockCallback.sol b/solidity/test/mocks/MockCallback.sol index 2a024193..e7060fcf 100644 --- a/solidity/test/mocks/MockCallback.sol +++ b/solidity/test/mocks/MockCallback.sol @@ -1,11 +1,10 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.19; -contract MockCallback { - uint256 public randomValue; +import {IProphetCallback} from '../../interfaces/IProphetCallback.sol'; - function callback(bytes32, /* _requestId */ bytes calldata _data) external { - uint256 _randomValue = abi.decode(_data, (uint256)); - randomValue = _randomValue; +contract MockCallback is IProphetCallback { + function prophetCallback(bytes calldata /* _callData */ ) external pure returns (bytes memory _callResponse) { + _callResponse = abi.encode(true); } } diff --git a/solidity/test/unit/modules/dispute/CircuitResolverModule.t.sol b/solidity/test/unit/modules/dispute/CircuitResolverModule.t.sol index 1dd489ae..a464dfca 100644 --- a/solidity/test/unit/modules/dispute/CircuitResolverModule.t.sol +++ b/solidity/test/unit/modules/dispute/CircuitResolverModule.t.sol @@ -14,6 +14,7 @@ import { ICircuitResolverModule } from '../../../../contracts/modules/dispute/CircuitResolverModule.sol'; +import {IProphetVerifier} from '../../../../interfaces/IProphetVerifier.sol'; import {IAccountingExtension} from '../../../../interfaces/extensions/IAccountingExtension.sol'; import {MockVerifier} from '../../../mocks/MockVerifier.sol'; @@ -136,8 +137,6 @@ contract CircuitResolverModule_Unit_DisputeResponse is BaseTest { uint256 _bondSize, bytes memory _callData ) public { - _callData = abi.encodeWithSelector(mockVerifier.calculateRoot.selector, _callData); - mockRequest.disputeModuleData = abi.encode( ICircuitResolverModule.RequestParameters({ callData: _callData, @@ -148,14 +147,18 @@ contract CircuitResolverModule_Unit_DisputeResponse is BaseTest { }) ); - bool _correctResponse = false; + bytes memory _correctResponse = abi.encode(false); mockResponse.requestId = _getId(mockRequest); mockDispute.requestId = mockResponse.requestId; mockDispute.responseId = _getId(mockResponse); // Mock and expect the call to the verifier - _mockAndExpect(address(mockVerifier), _callData, abi.encode(_correctResponse)); + _mockAndExpect( + address(mockVerifier), + abi.encodeWithSelector(IProphetVerifier.prophetVerify.selector, _callData), + abi.encode(_correctResponse) + ); // Mock and expect the call the oracle, updating the dispute's status _mockAndExpect( @@ -187,14 +190,17 @@ contract CircuitResolverModule_Unit_DisputeResponse is BaseTest { }) ); - bytes32 _requestId = _getId(mockRequest); - bool _correctResponse = false; + bytes memory _correctResponse = abi.encode(false); - mockResponse.requestId = _requestId; + mockResponse.requestId = _getId(mockRequest); mockResponse.response = abi.encode(true); // Mock and expect the call to the verifier - _mockAndExpect(address(mockVerifier), _callData, abi.encode(_correctResponse)); + _mockAndExpect( + address(mockVerifier), + abi.encodeWithSelector(IProphetVerifier.prophetVerify.selector, _callData), + abi.encode(_correctResponse) + ); // Mock and expect the call the oracle, updating the dispute's status _mockAndExpect( @@ -228,8 +234,6 @@ contract CircuitResolverModule_Unit_DisputeResponse is BaseTest { uint256 _bondSize, bytes memory _callData ) public { - _callData = abi.encodeWithSelector(mockVerifier.calculateRoot.selector, _callData); - mockRequest.disputeModuleData = abi.encode( ICircuitResolverModule.RequestParameters({ callData: _callData, @@ -240,13 +244,17 @@ contract CircuitResolverModule_Unit_DisputeResponse is BaseTest { }) ); - bytes memory _encodedCorrectResponse = abi.encode(true); + bytes memory _correctResponse = abi.encode(true); mockResponse.requestId = _getId(mockRequest); - mockResponse.response = _encodedCorrectResponse; + mockResponse.response = _correctResponse; // Mock and expect the call to the verifier - _mockAndExpect(address(mockVerifier), _callData, _encodedCorrectResponse); + _mockAndExpect( + address(mockVerifier), + abi.encodeWithSelector(IProphetVerifier.prophetVerify.selector, _callData), + abi.encode(_correctResponse) + ); // Mock and expect the call the oracle, updating the dispute's status _mockAndExpect( diff --git a/solidity/test/unit/modules/finality/CallbackModule.t.sol b/solidity/test/unit/modules/finality/CallbackModule.t.sol index 0e559d17..8654aef3 100644 --- a/solidity/test/unit/modules/finality/CallbackModule.t.sol +++ b/solidity/test/unit/modules/finality/CallbackModule.t.sol @@ -10,6 +10,8 @@ import {IOracle} from '@defi-wonderland/prophet-core-contracts/solidity/interfac import {CallbackModule, ICallbackModule} from '../../../../contracts/modules/finality/CallbackModule.sol'; +import {IProphetCallback} from '../../../../interfaces/IProphetCallback.sol'; + /** * @title Callback Module Unit tests */ @@ -84,6 +86,9 @@ contract CallbackModule_Unit_FinalizeRequest is BaseTest { mockRequest.finalityModuleData = abi.encode(ICallbackModule.RequestParameters({target: _target, data: _data})); mockResponse.requestId = _getId(mockRequest); + // Mock and expect the callback + _mockAndExpect(_target, abi.encodeWithSelector(IProphetCallback.prophetCallback.selector, _data), abi.encode('')); + // Check: is the event emitted? vm.expectEmit(true, true, true, true, address(callbackModule)); emit Callback(mockResponse.requestId, _target, _data); @@ -108,7 +113,7 @@ contract CallbackModule_Unit_FinalizeRequest is BaseTest { mockResponse.requestId = _getId(mockRequest); // Mock and expect the callback - _mockAndExpect(_target, _data, abi.encode('')); + _mockAndExpect(_target, abi.encodeWithSelector(IProphetCallback.prophetCallback.selector, _data), abi.encode('')); vm.prank(address(oracle)); callbackModule.finalizeRequest(mockRequest, mockResponse, _proposer); diff --git a/solidity/test/unit/modules/finality/MultipleCallbacksModule.t.sol b/solidity/test/unit/modules/finality/MultipleCallbacksModule.t.sol index 11f4907e..21a26a8c 100644 --- a/solidity/test/unit/modules/finality/MultipleCallbacksModule.t.sol +++ b/solidity/test/unit/modules/finality/MultipleCallbacksModule.t.sol @@ -13,6 +13,8 @@ import { MultipleCallbacksModule } from '../../../../contracts/modules/finality/MultipleCallbacksModule.sol'; +import {IProphetCallback} from '../../../../interfaces/IProphetCallback.sol'; + contract BaseTest is Test, Helpers { // The target contract MultipleCallbacksModule public multipleCallbackModule; @@ -102,7 +104,9 @@ contract MultipleCallbacksModule_Unit_FinalizeRequests is BaseTest { // Skip precompiles, VM, console.log addresses, etc _assumeFuzzable(_target); - _mockAndExpect(_target, _calldata, abi.encode()); + _mockAndExpect( + _target, abi.encodeWithSelector(IProphetCallback.prophetCallback.selector, _calldata), abi.encode('') + ); // Check: is the event emitted? vm.expectEmit(true, true, true, true, address(multipleCallbackModule));