From 7dd25d3a452d80def702949b9309e87ed1622acc Mon Sep 17 00:00:00 2001 From: xorsal Date: Wed, 25 Sep 2024 05:05:52 -0400 Subject: [PATCH] feat: add access control for authorized callers (#62) --- .../extensions/BondEscalationAccounting.sol | 21 +++- .../extensions/IBondEscalationAccounting.sol | 13 +++ solidity/scripts/Deploy.sol | 4 +- .../test/integration/BondEscalation.t.sol | 73 +++++++++++++- solidity/test/integration/IntegrationBase.sol | 4 +- .../dispute/BondEscalationAccounting.t.sol | 95 ++++++++++++++++--- 6 files changed, 186 insertions(+), 24 deletions(-) diff --git a/solidity/contracts/extensions/BondEscalationAccounting.sol b/solidity/contracts/extensions/BondEscalationAccounting.sol index 8e33764..cd0d4fb 100644 --- a/solidity/contracts/extensions/BondEscalationAccounting.sol +++ b/solidity/contracts/extensions/BondEscalationAccounting.sol @@ -19,7 +19,20 @@ contract BondEscalationAccounting is AccountingExtension, IBondEscalationAccount /// @inheritdoc IBondEscalationAccounting mapping(bytes32 _requestId => mapping(address _pledger => bool _claimed)) public pledgerClaimed; - constructor(IOracle _oracle) AccountingExtension(_oracle) {} + /// @inheritdoc IBondEscalationAccounting + mapping(address _caller => bool _authorized) public authorizedCallers; + + constructor(IOracle _oracle, address[] memory _authorizedCallers) AccountingExtension(_oracle) { + uint256 _length = _authorizedCallers.length; + for (uint256 _i; _i < _length; ++_i) { + authorizedCallers[_authorizedCallers[_i]] = true; + } + } + + modifier onlyAuthorizedCaller() { + if (!authorizedCallers[msg.sender]) revert BondEscalationAccounting_UnauthorizedCaller(); + _; + } /// @inheritdoc IBondEscalationAccounting function pledge( @@ -28,7 +41,7 @@ contract BondEscalationAccounting is AccountingExtension, IBondEscalationAccount IOracle.Dispute calldata _dispute, IERC20 _token, uint256 _amount - ) external { + ) external onlyAuthorizedCaller { bytes32 _requestId = _getId(_request); bytes32 _disputeId = _validateDispute(_request, _dispute); @@ -51,7 +64,7 @@ contract BondEscalationAccounting is AccountingExtension, IBondEscalationAccount IERC20 _token, uint256 _amountPerPledger, uint256 _winningPledgersLength - ) external { + ) external onlyAuthorizedCaller { bytes32 _requestId = _getId(_request); bytes32 _disputeId = _validateDispute(_request, _dispute); @@ -127,7 +140,7 @@ contract BondEscalationAccounting is AccountingExtension, IBondEscalationAccount address _pledger, IERC20 _token, uint256 _amount - ) external { + ) external onlyAuthorizedCaller { bytes32 _requestId = _getId(_request); bytes32 _disputeId = _validateDispute(_request, _dispute); diff --git a/solidity/interfaces/extensions/IBondEscalationAccounting.sol b/solidity/interfaces/extensions/IBondEscalationAccounting.sol index cf8f90c..f575462 100644 --- a/solidity/interfaces/extensions/IBondEscalationAccounting.sol +++ b/solidity/interfaces/extensions/IBondEscalationAccounting.sol @@ -109,6 +109,11 @@ interface IBondEscalationAccounting is IAccountingExtension { */ error BondEscalationAccounting_AlreadySettled(); + /** + * @notice Thrown when caller is not authorized + */ + error BondEscalationAccounting_UnauthorizedCaller(); + /*/////////////////////////////////////////////////////////////// STRUCTS //////////////////////////////////////////////////////////////*/ @@ -162,6 +167,14 @@ interface IBondEscalationAccounting is IAccountingExtension { */ function pledgerClaimed(bytes32 _requestId, address _pledger) external returns (bool _claimed); + /** + * @notice Checks whether an address is an authorized caller. + * + * @param _caller The address to check + * @return _authorized True if the address is authorized, false otherwise + */ + function authorizedCallers(address _caller) external returns (bool _authorized); + /*/////////////////////////////////////////////////////////////// LOGIC //////////////////////////////////////////////////////////////*/ diff --git a/solidity/scripts/Deploy.sol b/solidity/scripts/Deploy.sol index 74ca00e..53b511e 100644 --- a/solidity/scripts/Deploy.sol +++ b/solidity/scripts/Deploy.sol @@ -125,7 +125,9 @@ contract Deploy is Script { console.log('ACCOUNTING_EXTENSION:', address(accountingExtension)); // Deploy bond escalation accounting - bondEscalationAccounting = new BondEscalationAccounting(oracle); + address[] memory authorizedCallers = new address[](1); + authorizedCallers[0] = address(bondEscalationModule); + bondEscalationAccounting = new BondEscalationAccounting(oracle, authorizedCallers); console.log('BOND_ESCALATION_ACCOUNTING_EXTENSION:', address(bondEscalationAccounting)); // Deploy circuit resolver module diff --git a/solidity/test/integration/BondEscalation.t.sol b/solidity/test/integration/BondEscalation.t.sol index e97dd75..12cfc82 100644 --- a/solidity/test/integration/BondEscalation.t.sol +++ b/solidity/test/integration/BondEscalation.t.sol @@ -7,6 +7,7 @@ contract Integration_BondEscalation is IntegrationBase { address internal _secondDisputer = makeAddr('secondDisputer'); address internal _secondProposer = makeAddr('secondProposer'); address internal _thirdProposer = makeAddr('thirdProposer'); + address internal _attacker = makeAddr('attacker'); bytes internal _responseData = abi.encode('response'); @@ -25,6 +26,11 @@ contract Integration_BondEscalation is IntegrationBase { _expectedDeadline = block.timestamp + 10 days; _bondEscalationDeadline = block.timestamp + 5 days; + setUpRequest(); + setUpEscalation(); + } + + function setUpRequest() public { mockRequest.requestModuleData = abi.encode( IHttpRequestModule.RequestParameters({ url: _expectedUrl, @@ -59,9 +65,12 @@ contract Integration_BondEscalation is IntegrationBase { ); mockRequest.disputeModule = address(_bondEscalationModule); + mockRequest.nonce = uint96(oracle.totalRequestCount()); _resetMockIds(); + } + function setUpEscalation() public { // Set up all approvals vm.prank(requester); _bondEscalationAccounting.approveModule(address(_requestModule)); @@ -470,8 +479,6 @@ contract Integration_BondEscalation is IntegrationBase { ////////////////// NEW MALICIOUS REQUEST //////////////////////// - address _attacker = makeAddr('attacker'); - mockRequest.nonce += 1; mockRequest.requester = _attacker; mockRequest.disputeModule = _attacker; @@ -486,8 +493,66 @@ contract Integration_BondEscalation is IntegrationBase { }) ); - uint256 _attackerBalance = _bondEscalationAccounting.balanceOf(_attacker, usdc); - assertEq(_attackerBalance, 0); + vm.startPrank(_attacker); + // Create a new proposal with another dispute module + _bondEscalationAccounting.approveModule(mockRequest.requestModule); + + vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_UnauthorizedCaller.selector); + _bondEscalationAccounting.releasePledge(mockRequest, mockDispute, _attacker, usdc, _pledgeSize * 4); + vm.stopPrank(); + } + + function test_authorizedAttackerAllowedModules() public { + // redeploy BondEscalationAccounting authorizing the attacker + address[] memory _authorizedCallers = new address[](3); + _authorizedCallers[0] = address(_bondEscalationModule); + _authorizedCallers[1] = _attacker; + _bondEscalationAccounting = new BondEscalationAccounting(oracle, _authorizedCallers); + + label(address(_bondEscalationAccounting), 'BondEscalationModule'); + setUpRequest(); + setUpEscalation(); + + ////////////////// DISPUTE ESCALATION //////////////////////// + // Step 1: Proposer pledges against the dispute + _deposit(_bondEscalationAccounting, proposer, usdc, _pledgeSize); + vm.prank(proposer); + _bondEscalationModule.pledgeAgainstDispute(mockRequest, mockDispute); + + // Step 2: Disputer doubles down + _deposit(_bondEscalationAccounting, disputer, usdc, _pledgeSize); + vm.prank(disputer); + _bondEscalationModule.pledgeForDispute(mockRequest, mockDispute); + + // Step 3: Proposer doubles down + _deposit(_bondEscalationAccounting, proposer, usdc, _pledgeSize); + vm.prank(proposer); + _bondEscalationModule.pledgeAgainstDispute(mockRequest, mockDispute); + + // Step 4: Disputer runs out of capital + // Step 5: The tying buffer kicks in + vm.warp(_bondEscalationDeadline + 1); + + // Step 6: An external party sees that Proposer's response is incorrect, so they bond the required WETH + _deposit(_bondEscalationAccounting, _secondDisputer, usdc, _pledgeSize); + vm.prank(_secondDisputer); + _bondEscalationModule.pledgeForDispute(mockRequest, mockDispute); + + ////////////////// NEW MALICIOUS REQUEST //////////////////////// + + mockRequest.nonce += 1; + mockRequest.requester = _attacker; + mockRequest.disputeModule = _attacker; + mockRequest.requestModuleData = abi.encode( + IHttpRequestModule.RequestParameters({ + url: _expectedUrl, + body: _expectedBody, + method: _expectedMethod, + accountingExtension: _bondEscalationAccounting, + paymentToken: usdc, + paymentAmount: 0 + }) + ); vm.startPrank(_attacker); // Create a new proposal with another dispute module diff --git a/solidity/test/integration/IntegrationBase.sol b/solidity/test/integration/IntegrationBase.sol index 04560e4..0cbd2f2 100644 --- a/solidity/test/integration/IntegrationBase.sol +++ b/solidity/test/integration/IntegrationBase.sol @@ -125,7 +125,9 @@ contract IntegrationBase is DSTestPlus, TestConstants, Helpers { _bondEscalationModule = new BondEscalationModule(oracle); label(address(_bondEscalationModule), 'BondEscalationModule'); - _bondEscalationAccounting = new BondEscalationAccounting(oracle); + address[] memory _authorizedCallers = new address[](1); + _authorizedCallers[0] = address(_bondEscalationModule); + _bondEscalationAccounting = new BondEscalationAccounting(oracle, _authorizedCallers); label(address(_bondEscalationAccounting), 'BondEscalationAccounting'); _mockCallback = new MockCallback(); diff --git a/solidity/test/unit/modules/dispute/BondEscalationAccounting.t.sol b/solidity/test/unit/modules/dispute/BondEscalationAccounting.t.sol index e4e4c13..fc2e74c 100644 --- a/solidity/test/unit/modules/dispute/BondEscalationAccounting.t.sol +++ b/solidity/test/unit/modules/dispute/BondEscalationAccounting.t.sol @@ -17,7 +17,10 @@ import {IAccountingExtension} from '../../../../interfaces/extensions/IAccountin import {Strings} from '@openzeppelin/contracts/utils/Strings.sol'; contract ForTest_BondEscalationAccounting is BondEscalationAccounting { - constructor(IOracle _oracle) BondEscalationAccounting(_oracle) {} + constructor( + IOracle _oracle, + address[] memory _authorizedCallers + ) BondEscalationAccounting(_oracle, _authorizedCallers) {} function forTest_setPledge(bytes32 _disputeId, IERC20 _token, uint256 _amount) public { pledges[_disputeId][_token] = _amount; @@ -65,6 +68,9 @@ contract BaseTest is Test, Helpers { address public bonder = makeAddr('bonder'); address public pledger = makeAddr('pledger'); + address public authorizedCaller = makeAddr('authorizedCaller'); + address public unauthorizedCaller = makeAddr('unauthorizedCaller'); + // Pledged Event event Pledged( address indexed _pledger, bytes32 indexed _requestId, bytes32 indexed _disputeId, IERC20 _token, uint256 _amount @@ -105,22 +111,41 @@ contract BaseTest is Test, Helpers { token = IERC20(makeAddr('ERC20')); vm.etch(address(token), hex'069420'); - bondEscalationAccounting = new ForTest_BondEscalationAccounting(oracle); + address[] memory _authorizedCallers = new address[](1); + _authorizedCallers[0] = authorizedCaller; + bondEscalationAccounting = new ForTest_BondEscalationAccounting(oracle, _authorizedCallers); } } contract BondEscalationAccounting_Unit_Pledge is BaseTest { + function test_revertIfCallerIsUnauthorized(address _pledger, uint256 _amount) public { + IOracle.Dispute memory _dispute = _getDispute(mockRequest, _getResponse(mockRequest, proposer)); + + // Check: does it revert if the caller is not authorized? + vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_UnauthorizedCaller.selector); + + vm.prank(unauthorizedCaller); + bondEscalationAccounting.pledge({ + _pledger: _pledger, + _request: mockRequest, + _dispute: _dispute, + _token: token, + _amount: _amount + }); + } + function test_revertIfDisallowedModule(address _pledger, uint256 _amount) public { (, IOracle.Dispute memory _dispute) = _getResponseAndDispute(oracle); // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_getId(mockRequest), address(this))), abi.encode(false) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_getId(mockRequest), authorizedCaller)), abi.encode(false) ); // Check: does it revert if called by an unauthorized module? vm.expectRevert(IAccountingExtension.AccountingExtension_UnauthorizedModule.selector); + vm.prank(authorizedCaller); bondEscalationAccounting.pledge({ _pledger: _pledger, _request: mockRequest, @@ -137,12 +162,13 @@ contract BondEscalationAccounting_Unit_Pledge is BaseTest { bytes32 _requestId = _getId(mockRequest); _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); // Check: does it revert if the pledger doesn't have enough deposited? vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_InsufficientFunds.selector); + vm.prank(authorizedCaller); bondEscalationAccounting.pledge({ _pledger: _pledger, _request: mockRequest, @@ -159,7 +185,7 @@ contract BondEscalationAccounting_Unit_Pledge is BaseTest { // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); bondEscalationAccounting.forTest_setBalanceOf(_pledger, token, _amount); @@ -171,6 +197,7 @@ contract BondEscalationAccounting_Unit_Pledge is BaseTest { uint256 _balanceBeforePledge = bondEscalationAccounting.balanceOf(_pledger, token); uint256 _pledgesBeforePledge = bondEscalationAccounting.pledges(_disputeId, token); + vm.prank(authorizedCaller); bondEscalationAccounting.pledge({ _pledger: _pledger, _request: mockRequest, @@ -190,18 +217,35 @@ contract BondEscalationAccounting_Unit_Pledge is BaseTest { } contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { + function test_revertIfCallerIsUnauthorized(uint256 _amountPerPledger, uint256 _numOfWinningPledgers) public { + IOracle.Dispute memory _dispute = _getDispute(mockRequest, _getResponse(mockRequest, proposer)); + + vm.startPrank(unauthorizedCaller); + // Check: does it revert if the caller is not authorized? + vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_UnauthorizedCaller.selector); + + bondEscalationAccounting.onSettleBondEscalation({ + _request: mockRequest, + _dispute: _dispute, + _token: token, + _amountPerPledger: _amountPerPledger, + _winningPledgersLength: _numOfWinningPledgers + }); + } + function test_revertIfDisallowedModule(uint256 _numOfWinningPledgers, uint256 _amountPerPledger) public { (, IOracle.Dispute memory _dispute) = _getResponseAndDispute(oracle); bytes32 _requestId = _getId(mockRequest); // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(false) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(false) ); // Check: does it revert if the module is not allowed? vm.expectRevert(IAccountingExtension.AccountingExtension_UnauthorizedModule.selector); + vm.prank(authorizedCaller); bondEscalationAccounting.onSettleBondEscalation({ _request: mockRequest, _dispute: _dispute, @@ -221,7 +265,7 @@ contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); // Mock and expect the call to oracle checking if the dispute exists @@ -236,6 +280,7 @@ contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { // Check: does it revert if the escalation is already settled? vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_AlreadySettled.selector); + vm.prank(authorizedCaller); bondEscalationAccounting.onSettleBondEscalation({ _request: mockRequest, _dispute: mockDispute, @@ -256,7 +301,7 @@ contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); address[] memory _winningPledgers = _createWinningPledgersArray(_numOfWinningPledgers); @@ -269,6 +314,7 @@ contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { // Check: does it revert if the pledger does not have enough funds? vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_InsufficientFunds.selector); + vm.prank(authorizedCaller); bondEscalationAccounting.onSettleBondEscalation({ _request: mockRequest, _dispute: _dispute, @@ -289,7 +335,7 @@ contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); address[] memory _winningPledgers = _createWinningPledgersArray(_numOfWinningPledgers); @@ -301,6 +347,7 @@ contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { vm.expectEmit(true, true, true, true, address(bondEscalationAccounting)); emit BondEscalationSettled(_requestId, _disputeId, token, _amountPerPledger, _numOfWinningPledgers); + vm.prank(authorizedCaller); bondEscalationAccounting.onSettleBondEscalation({ _request: mockRequest, _dispute: _dispute, @@ -320,23 +367,40 @@ contract BondEscalationAccounting_Unit_OnSettleBondEscalation is BaseTest { assertEq(_requestIdSaved, _requestId); assertEq(address(_token), address(token)); assertEq(_amountPerPledgerSaved, _amountPerPledger); - assertEq(address(_bondEscalationModule), address(this)); + assertEq(address(_bondEscalationModule), authorizedCaller); } } contract BondEscalationAccounting_Unit_ReleasePledge is BaseTest { + function test_revertIfCallerIsUnauthorized(uint256 _amount, address _pledger) public { + IOracle.Dispute memory _dispute = _getDispute(mockRequest, _getResponse(mockRequest, proposer)); + + // Check: does it revert if the caller is not authorized? + vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_UnauthorizedCaller.selector); + + vm.prank(unauthorizedCaller); + bondEscalationAccounting.releasePledge({ + _request: mockRequest, + _dispute: _dispute, + _pledger: _pledger, + _token: token, + _amount: _amount + }); + } + function test_revertIfDisallowedModule(address _pledger, uint256 _amount) public { (, IOracle.Dispute memory _dispute) = _getResponseAndDispute(oracle); bytes32 _requestId = _getId(mockRequest); // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(false) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(false) ); // Check: does it revert if the module is not authorized? vm.expectRevert(IAccountingExtension.AccountingExtension_UnauthorizedModule.selector); + vm.prank(authorizedCaller); bondEscalationAccounting.releasePledge({ _request: mockRequest, _dispute: _dispute, @@ -355,7 +419,7 @@ contract BondEscalationAccounting_Unit_ReleasePledge is BaseTest { // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); bondEscalationAccounting.forTest_setPledge(_disputeId, token, _amount); @@ -364,6 +428,7 @@ contract BondEscalationAccounting_Unit_ReleasePledge is BaseTest { // Check: does it revert if the pledger does not have enough funds pledged? vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_InsufficientFunds.selector); + vm.prank(authorizedCaller); bondEscalationAccounting.releasePledge({ _request: mockRequest, _dispute: _dispute, @@ -380,11 +445,12 @@ contract BondEscalationAccounting_Unit_ReleasePledge is BaseTest { // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); bondEscalationAccounting.forTest_setPledge(_disputeId, token, _amount); + vm.prank(authorizedCaller); bondEscalationAccounting.releasePledge({ _request: mockRequest, _dispute: _dispute, @@ -404,7 +470,7 @@ contract BondEscalationAccounting_Unit_ReleasePledge is BaseTest { // Mock and expect the call to oracle checking if the module is allowed _mockAndExpect( - address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, address(this))), abi.encode(true) + address(oracle), abi.encodeCall(IOracle.allowedModule, (_requestId, authorizedCaller)), abi.encode(true) ); bondEscalationAccounting.forTest_setPledge(_disputeId, token, _amount); @@ -413,6 +479,7 @@ contract BondEscalationAccounting_Unit_ReleasePledge is BaseTest { vm.expectEmit(true, true, true, true, address(bondEscalationAccounting)); emit PledgeReleased(_requestId, _disputeId, _pledger, token, _amount); + vm.prank(authorizedCaller); bondEscalationAccounting.releasePledge({ _request: mockRequest, _dispute: _dispute,