fix: wond-15 (#54)

* fix: wond-15

* fix: correct validation

* fix: validate dispute module allowance

* fix: validate dispute module allowance

* fix: change prank and correct prank

* fix: comment

* test: units

* fix: error naming

* fix: pr

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",

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;

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);
   }

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

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);
+  }
 }

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();
 

solidity/test/unit/extensions/AccountingExtension.t.sol

@@ -253,20 +253,15 @@ 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));
 
     // 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 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 {

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,22 +129,19 @@ 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(
     IERC20 _token,
     uint256 _bondSize,
     uint256 _deadline,
     uint256 _disputeWindow,
-    address _sender,
     address _proposer
   ) public {
     _deadline = bound(_deadline, block.timestamp + 1, type(uint248).max);
@@ -163,16 +159,53 @@ 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()
     );
 
     // Check: is the event emitted?
     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);
   }

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"