diff --git a/package.json b/package.json index 3971c681..1e1f39c8 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ "@defi-wonderland/solidity-utils": "0.0.0-3e9c8e8b", "dotenv-cli": "7.2.1", "ds-test": "https://github.com/dapphub/ds-test.git#e282159d5170298eb2455a6c05280ab5a73a4ef0", - "forge-std": "https://github.com/foundry-rs/forge-std.git#f73c73d2018eb6a111f35e4dae7b4f27401e9421", + "forge-std": "https://git@github.com/foundry-rs/forge-std.git#1.9.1", "husky": "8.0.3", "lint-staged": "13.2.2", "pinst": "3.0.0", diff --git a/solidity/contracts/extensions/AccountingExtension.sol b/solidity/contracts/extensions/AccountingExtension.sol index 87743529..790b507b 100644 --- a/solidity/contracts/extensions/AccountingExtension.sol +++ b/solidity/contracts/extensions/AccountingExtension.sol @@ -106,7 +106,7 @@ contract AccountingExtension is IAccountingExtension { IERC20 _token, uint256 _amount ) external onlyAllowedModule(_requestId) onlyParticipant(_requestId, _bonder) { - if (!_approvals[_bonder].contains(msg.sender)) revert AccountingExtension_InsufficientAllowance(); + if (!_approvals[_bonder].contains(msg.sender)) revert AccountingExtension_NotAllowed(); if (balanceOf[_bonder][_token] < _amount) revert AccountingExtension_InsufficientFunds(); bondedAmountOf[_bonder][_token][_requestId] += _amount; @@ -126,9 +126,13 @@ contract AccountingExtension is IAccountingExtension { uint256 _amount, address _sender ) external onlyAllowedModule(_requestId) onlyParticipant(_requestId, _bonder) { - if (!(_approvals[_bonder].contains(msg.sender) || _approvals[_bonder].contains(_sender))) { - revert AccountingExtension_InsufficientAllowance(); + bool _moduleApproved = _approvals[_bonder].contains(msg.sender); + bool _senderApproved = _approvals[_bonder].contains(_sender); + + if (!(_moduleApproved && _senderApproved)) { + revert AccountingExtension_NotAllowed(); } + if (balanceOf[_bonder][_token] < _amount) revert AccountingExtension_InsufficientFunds(); bondedAmountOf[_bonder][_token][_requestId] += _amount; diff --git a/solidity/contracts/modules/response/BondedResponseModule.sol b/solidity/contracts/modules/response/BondedResponseModule.sol index 8794169e..6a55b75a 100644 --- a/solidity/contracts/modules/response/BondedResponseModule.sol +++ b/solidity/contracts/modules/response/BondedResponseModule.sol @@ -45,13 +45,22 @@ contract BondedResponseModule is Module, IBondedResponseModule { if (_status == IOracle.DisputeStatus.Lost) revert BondedResponseModule_AlreadyResponded(); } - _params.accountingExtension.bond({ - _bonder: _response.proposer, - _requestId: _response.requestId, - _token: _params.bondToken, - _amount: _params.bondSize, - _sender: _sender - }); + if (_sender != _response.proposer) { + _params.accountingExtension.bond({ + _bonder: _response.proposer, + _requestId: _response.requestId, + _token: _params.bondToken, + _amount: _params.bondSize, + _sender: _sender + }); + } else { + _params.accountingExtension.bond({ + _bonder: _response.proposer, + _requestId: _response.requestId, + _token: _params.bondToken, + _amount: _params.bondSize + }); + } emit ResponseProposed(_response.requestId, _response, block.number); } diff --git a/solidity/interfaces/extensions/IAccountingExtension.sol b/solidity/interfaces/extensions/IAccountingExtension.sol index ad2d11bf..20fe9f62 100644 --- a/solidity/interfaces/extensions/IAccountingExtension.sol +++ b/solidity/interfaces/extensions/IAccountingExtension.sol @@ -78,7 +78,7 @@ interface IAccountingExtension { /** * @notice Thrown when the module bonding user tokens hasn't been approved by the user. */ - error AccountingExtension_InsufficientAllowance(); + error AccountingExtension_NotAllowed(); /** * @notice Thrown when an `onlyAllowedModule` function is called by something diff --git a/solidity/test/integration/ResponseProposal.t.sol b/solidity/test/integration/ResponseProposal.t.sol index f79f2db5..eaee2adc 100644 --- a/solidity/test/integration/ResponseProposal.t.sol +++ b/solidity/test/integration/ResponseProposal.t.sol @@ -130,4 +130,88 @@ contract Integration_ResponseProposal is IntegrationBase { vm.prank(proposer); oracle.proposeResponse(mockRequest, mockResponse); } + + /** + * @notice Proposing from an approved dispute module + */ + function test_proposeResponse_fromApprovedDisputeModule(bytes memory _responseBytes) public { + address _otherRequester = makeAddr('otherRequester'); + address _approvedDisputeModule = makeAddr('_approvedDisputeModule'); + + // Approve the new dispute module + vm.prank(proposer); + _accountingExtension.approveModule(_approvedDisputeModule); + + mockRequest.nonce += 1; + mockRequest.requester = _otherRequester; + mockRequest.disputeModule = _approvedDisputeModule; + mockRequest.requestModuleData = abi.encode( + IHttpRequestModule.RequestParameters({ + url: _expectedUrl, + body: _expectedBody, + method: _expectedMethod, + accountingExtension: _accountingExtension, + paymentToken: usdc, + paymentAmount: 0 + }) + ); + + uint256 _oldProposerBalance = _accountingExtension.balanceOf(proposer, usdc); + assertGt(_oldProposerBalance, 0); + + vm.startPrank(_otherRequester); + // Create a new request with another dispute module + _accountingExtension.approveModule(mockRequest.requestModule); + bytes32 _requestIdApprovedDisputeModule = oracle.createRequest(mockRequest, _ipfsHash); + + changePrank(_approvedDisputeModule); + + // Propose a response from the approved dispute module + mockResponse.response = _responseBytes; + mockResponse.proposer = proposer; + mockResponse.requestId = _requestIdApprovedDisputeModule; + + oracle.proposeResponse(mockRequest, mockResponse); + vm.stopPrank(); + + uint256 _newProposerBalance = _accountingExtension.balanceOf(proposer, usdc); + + // Proposer got their balance bonded when they didn't create the response + assertTrue(_expectedBondSize != 0); + assertEq(_oldProposerBalance, _newProposerBalance + _expectedBondSize); + } + + /** + * @notice Proposing from an unapproved dispute module + */ + function test_proposeResponse_fromUnapprovedDisputeModule(bytes memory _responseBytes) public { + address _attacker = makeAddr('attacker'); + mockRequest.nonce += 1; + mockRequest.requester = _attacker; + mockRequest.requestModuleData = abi.encode( + IHttpRequestModule.RequestParameters({ + url: _expectedUrl, + body: _expectedBody, + method: _expectedMethod, + accountingExtension: _accountingExtension, + paymentToken: usdc, + paymentAmount: 0 + }) + ); + + vm.startPrank(_attacker); + // Attacker creates a request with their own address as the dispute module + mockRequest.disputeModule = _attacker; + _accountingExtension.approveModule(mockRequest.requestModule); + bytes32 _requestIdAttacker = oracle.createRequest(mockRequest, _ipfsHash); + + // Attacker proposes a response from their address (the dispute module) and using another user as the proposer + mockResponse.response = _responseBytes; + mockResponse.proposer = proposer; + mockResponse.requestId = _requestIdAttacker; + + // Should revert as the dispute module is not approved + vm.expectRevert(IAccountingExtension.AccountingExtension_NotAllowed.selector); + oracle.proposeResponse(mockRequest, mockResponse); + } } diff --git a/solidity/test/integration/RootVerification.t.sol b/solidity/test/integration/RootVerification.t.sol index b84b32b5..e35236e8 100644 --- a/solidity/test/integration/RootVerification.t.sol +++ b/solidity/test/integration/RootVerification.t.sol @@ -133,6 +133,7 @@ contract Integration_RootVerification is IntegrationBase { vm.startPrank(disputer); _accountingExtension.approveModule(address(_responseModule)); + _accountingExtension.approveModule(address(mockRequest.disputeModule)); oracle.disputeResponse(mockRequest, mockResponse, mockDispute); vm.stopPrank(); diff --git a/solidity/test/unit/extensions/AccountingExtension.t.sol b/solidity/test/unit/extensions/AccountingExtension.t.sol index c5be47a1..13aecead 100644 --- a/solidity/test/unit/extensions/AccountingExtension.t.sol +++ b/solidity/test/unit/extensions/AccountingExtension.t.sol @@ -253,12 +253,7 @@ contract AccountingExtension_Unit_Bond is BaseTest { /** * @notice Test bonding reverting if the module is not approved to bond the _bonder funds */ - function test_revertIfInsufficientAllowance( - bytes32 _requestId, - uint256 _amount, - address _bonder, - address _module - ) public { + function test_revertIfNotAllowed(bytes32 _requestId, uint256 _amount, address _bonder, address _module) public { // Mock and expect the module calling validation _mockAndExpect(address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, _module)), abi.encode(true)); @@ -266,7 +261,7 @@ contract AccountingExtension_Unit_Bond is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.isParticipant, (_requestId, _bonder)), abi.encode(true)); // Check: does it revert if the module is not approved? - vm.expectRevert(IAccountingExtension.AccountingExtension_InsufficientAllowance.selector); + vm.expectRevert(IAccountingExtension.AccountingExtension_NotAllowed.selector); vm.prank(_module); extension.bond({_bonder: _bonder, _requestId: _requestId, _token: token, _amount: _amount}); @@ -275,7 +270,7 @@ contract AccountingExtension_Unit_Bond is BaseTest { /** * @notice Test bonding reverting if the caller is not approved to bond the _bonder funds */ - function test_withCaller_revertIfInsufficientAllowance( + function test_withCaller_revertIfNotAllowed( bytes32 _requestId, uint256 _amount, address _bonder, @@ -288,11 +283,67 @@ contract AccountingExtension_Unit_Bond is BaseTest { _mockAndExpect(address(oracle), abi.encodeCall(IOracle.isParticipant, (_requestId, _bonder)), abi.encode(true)); // Check: does it revert if the caller is not approved? - vm.expectRevert(IAccountingExtension.AccountingExtension_InsufficientAllowance.selector); + vm.expectRevert(IAccountingExtension.AccountingExtension_NotAllowed.selector); vm.prank(_sender); extension.bond({_bonder: _bonder, _requestId: _requestId, _token: token, _amount: _amount, _sender: _sender}); } + + /** + * @notice Test bonding reverting if only the sender is approved but not the module + */ + function test_withCaller_revertIfNotAllowed_onlySender( + bytes32 _requestId, + uint256 _amount, + address _bonder, + address _sender, + address _module + ) public { + vm.assume(_sender != _module); + + vm.prank(_bonder); + extension.approveModule(_sender); + + // mock the module calling validation + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, _module)), abi.encode(true)); + + // Mock and expect the module checking for a participant + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.isParticipant, (_requestId, _bonder)), abi.encode(true)); + + // Check: does it revert if the caller is not approved? + vm.expectRevert(IAccountingExtension.AccountingExtension_NotAllowed.selector); + + vm.prank(_module); + extension.bond({_bonder: _bonder, _requestId: _requestId, _token: token, _amount: _amount, _sender: _sender}); + } + + /** + * @notice Test bonding reverting if only the module is approved but not the sender + */ + function test_withCaller_revertIfNotAllowed_onlyModule( + bytes32 _requestId, + uint256 _amount, + address _bonder, + address _sender, + address _module + ) public { + vm.assume(_sender != _module); + + vm.prank(_bonder); + extension.approveModule(_module); + + // mock the module calling validation + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, _module)), abi.encode(true)); + + // Mock and expect the module checking for a participant + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.isParticipant, (_requestId, _bonder)), abi.encode(true)); + + // Check: does it revert if the caller is not approved? + vm.expectRevert(IAccountingExtension.AccountingExtension_NotAllowed.selector); + + vm.prank(_module); + extension.bond({_bonder: _bonder, _requestId: _requestId, _token: token, _amount: _amount, _sender: _sender}); + } } contract AccountingExtension_Unit_Pay is BaseTest { diff --git a/solidity/test/unit/modules/response/BondedResponseModule.t.sol b/solidity/test/unit/modules/response/BondedResponseModule.t.sol index 73f95a0f..71d74ea0 100644 --- a/solidity/test/unit/modules/response/BondedResponseModule.t.sol +++ b/solidity/test/unit/modules/response/BondedResponseModule.t.sol @@ -110,9 +110,8 @@ contract BondedResponseModule_Unit_Propose is BaseTest { uint256 _bondSize, uint256 _deadline, uint256 _disputeWindow, - address _sender, address _proposer - ) public assumeFuzzable(_sender) assumeFuzzable(_proposer) { + ) public assumeFuzzable(_proposer) { _deadline = bound(_deadline, block.timestamp + 1, type(uint248).max); _disputeWindow = bound(_disputeWindow, 61, 365 days); _bondSize = bound(_bondSize, 0, type(uint248).max); @@ -130,14 +129,12 @@ contract BondedResponseModule_Unit_Propose is BaseTest { // Mock and expect IAccountingExtension.bond to be called _mockAndExpect( address(accounting), - abi.encodeWithSignature( - 'bond(address,bytes32,address,uint256,address)', _proposer, _requestId, _token, _bondSize, _sender - ), + abi.encodeWithSignature('bond(address,bytes32,address,uint256)', _proposer, _requestId, _token, _bondSize), abi.encode() ); vm.prank(address(oracle)); - bondedResponseModule.propose(mockRequest, mockResponse, _sender); + bondedResponseModule.propose(mockRequest, mockResponse, _proposer); } function test_emitsEvent( @@ -145,7 +142,6 @@ contract BondedResponseModule_Unit_Propose is BaseTest { uint256 _bondSize, uint256 _deadline, uint256 _disputeWindow, - address _sender, address _proposer ) public { _deadline = bound(_deadline, block.timestamp + 1, type(uint248).max); @@ -163,9 +159,7 @@ contract BondedResponseModule_Unit_Propose is BaseTest { // Mock and expect IOracle.getResponseIds to be called _mockAndExpect( address(accounting), - abi.encodeWithSignature( - 'bond(address,bytes32,address,uint256,address)', _proposer, _requestId, _token, _bondSize, _sender - ), + abi.encodeWithSignature('bond(address,bytes32,address,uint256)', _proposer, _requestId, _token, _bondSize), abi.encode() ); @@ -173,6 +167,45 @@ contract BondedResponseModule_Unit_Propose is BaseTest { vm.expectEmit(true, true, true, true, address(bondedResponseModule)); emit ResponseProposed({_requestId: _requestId, _response: mockResponse, _blockNumber: block.number}); + vm.prank(address(oracle)); + bondedResponseModule.propose(mockRequest, mockResponse, _proposer); + } + + /** + * @notice Test that the propose function works correctly and bonds the proposer's funds when the sender is different than the proposer + */ + function test_propose_another_sender( + IERC20 _token, + uint256 _bondSize, + uint256 _deadline, + uint256 _disputeWindow, + address _sender, + address _proposer + ) public assumeFuzzable(_sender) assumeFuzzable(_proposer) { + vm.assume(_sender != _proposer); + _deadline = bound(_deadline, block.timestamp + 1, type(uint248).max); + _disputeWindow = bound(_disputeWindow, 61, 365 days); + _bondSize = bound(_bondSize, 0, type(uint248).max); + + // Set the response module parameters + mockRequest.responseModuleData = abi.encode(accounting, _token, _bondSize, _deadline, _disputeWindow); + + bytes32 _requestId = _getId(mockRequest); + mockResponse.requestId = _requestId; + mockResponse.proposer = _proposer; + + // Mock and expect IOracle.getResponseIds to be called + _mockAndExpect(address(oracle), abi.encodeCall(IOracle.getResponseIds, _requestId), abi.encode(new bytes32[](0))); + + // Mock and expect IAccountingExtension.bond to be called + _mockAndExpect( + address(accounting), + abi.encodeWithSignature( + 'bond(address,bytes32,address,uint256,address)', _proposer, _requestId, _token, _bondSize, _sender + ), + abi.encode() + ); + vm.prank(address(oracle)); bondedResponseModule.propose(mockRequest, mockResponse, _sender); } diff --git a/yarn.lock b/yarn.lock index 8ee18c05..be7ec18e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1051,9 +1051,9 @@ dotgitignore@^2.1.0: find-up "^3.0.0" minimatch "^3.0.4" -"ds-test@git+https://github.com/dapphub/ds-test.git": +"ds-test@https://github.com/dapphub/ds-test": version "1.0.0" - resolved "git+https://github.com/dapphub/ds-test.git#e282159d5170298eb2455a6c05280ab5a73a4ef0" + resolved "https://github.com/dapphub/ds-test#e282159d5170298eb2455a6c05280ab5a73a4ef0" "ds-test@https://github.com/dapphub/ds-test.git#e282159d5170298eb2455a6c05280ab5a73a4ef0": version "1.0.0" @@ -1356,13 +1356,13 @@ flatted@^2.0.0: resolved "https://registry.yarnpkg.com/flatted/-/flatted-2.0.2.tgz#4575b21e2bcee7434aa9be662f4b7b5f9c2b5138" integrity sha512-r5wGx7YeOwNWNlCA0wQ86zKyDLMQr+/RB8xy74M4hTphfmjlijTSSXGuH8rnvKZnfT9i+75zmd8jcKdMR4O6jA== -"forge-std@git+https://github.com/foundry-rs/forge-std.git": +"forge-std@https://git@github.com/foundry-rs/forge-std.git#1.9.1": version "1.9.1" - resolved "git+https://github.com/foundry-rs/forge-std.git#c28115db8d90ebffb41953cf83aac63130f4bd40" + resolved "https://git@github.com/foundry-rs/forge-std.git#07263d193d621c4b2b0ce8b4d54af58f6957d97d" -"forge-std@https://github.com/foundry-rs/forge-std.git#f73c73d2018eb6a111f35e4dae7b4f27401e9421": - version "1.7.1" - resolved "https://github.com/foundry-rs/forge-std.git#f73c73d2018eb6a111f35e4dae7b4f27401e9421" +"forge-std@https://github.com/foundry-rs/forge-std": + version "1.9.1" + resolved "https://github.com/foundry-rs/forge-std#c28115db8d90ebffb41953cf83aac63130f4bd40" fs-extra@^11.0.0: version "11.2.0"