Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add access control for authorized callers #62

Merged
merged 13 commits into from
Sep 25, 2024
Merged

feat: add access control for authorized callers #62

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a07df6d
feat: add authorized callers for BondEscalationAccouning
xorsal Sep 19, 2024
e73a1f8
test: update tests to use authorized callers
xorsal Sep 19, 2024
61dd9bd
test: pass authorized callers param to BondEscalationAccounting
xorsal Sep 19, 2024
5a20ed9
docs: update docs
xorsal Sep 19, 2024
04eafc8
chore: run linter
xorsal Sep 19, 2024
fcd78bc
test: address PR review comments, remove unnecessary assumes
xorsal Sep 19, 2024
52353e5
feat: add onlyAuthorizedCaller modifier
xorsal Sep 20, 2024
4545d9e
test: use mocked dispute
xorsal Sep 20, 2024
cf6d1de
test: now reverts because the caller is unauthorized
xorsal Sep 20, 2024
9f4dabc
test: get rid of unnecessary assert
xorsal Sep 24, 2024
c92c1e6
chore: extract request and escalation setup to a fn
xorsal Sep 25, 2024
4e155a2
test: copy attackerAllowedModules test case
xorsal Sep 25, 2024
c149038
test: redeploy bond escalation accounting
xorsal Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions solidity/contracts/extensions/BondEscalationAccounting.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
xorsal marked this conversation as resolved.
Show resolved Hide resolved
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(
Expand All @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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);

Expand Down
13 changes: 13 additions & 0 deletions solidity/interfaces/extensions/IBondEscalationAccounting.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ interface IBondEscalationAccounting is IAccountingExtension {
*/
error BondEscalationAccounting_AlreadySettled();

/**
* @notice Thrown when caller is not authorized
*/
error BondEscalationAccounting_UnauthorizedCaller();

/*///////////////////////////////////////////////////////////////
STRUCTS
//////////////////////////////////////////////////////////////*/
Expand Down Expand Up @@ -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
//////////////////////////////////////////////////////////////*/
Expand Down
4 changes: 3 additions & 1 deletion solidity/scripts/Deploy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment on lines +128 to +130
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note here that we don't have any integration tests for the BondEscalationResolutionModule, only for the dispute one. Hence we're not adding it to the list of authorized callers.

console.log('BOND_ESCALATION_ACCOUNTING_EXTENSION:', address(bondEscalationAccounting));

// Deploy circuit resolver module
Expand Down
2 changes: 1 addition & 1 deletion solidity/test/integration/BondEscalation.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ contract Integration_BondEscalation is IntegrationBase {
// Create a new proposal with another dispute module
_bondEscalationAccounting.approveModule(mockRequest.requestModule);

vm.expectRevert(ValidatorLib.ValidatorLib_InvalidDisputeBody.selector);
vm.expectRevert(IBondEscalationAccounting.BondEscalationAccounting_UnauthorizedCaller.selector);
Copy link
Member

@0xJabberwock 0xJabberwock Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the erased expectation of reversion should still be tested, since the attacker could theoretically be an authorized caller.

Copy link
Contributor Author

@xorsal xorsal Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
What do you think about mocking authorizedCallers immediately after the line you selected, something like this:

stdstore
  .target(address(_bondEscalationAccounting))
  .sig('authorizedCallers(address)')
  .with_key(_attacker)
  .checked_write(true);

vm.expectRevert(ValidatorLib.ValidatorLib_InvalidDisputeBody.selector);
_bondEscalationAccounting.releasePledge(mockRequest, mockDispute, _attacker, usdc, _pledgeSize * 4);

Copy link
Member

@0xJabberwock 0xJabberwock Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could work, although I'd try to avoid mocking in integration tests—a workaround could be to redeploy BondEscalationAccounting with the attacker as an authorized caller (maybe in a separate test case).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect 👌
Yes, I thought about that but redeploying the BondEscalationAccounting is somewhat cumbersome because integration tests rely on common variables like mockRequest.

_bondEscalationAccounting.releasePledge(mockRequest, mockDispute, _attacker, usdc, _pledgeSize * 4);
vm.stopPrank();
}
Expand Down
4 changes: 3 additions & 1 deletion solidity/test/integration/IntegrationBase.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
95 changes: 81 additions & 14 deletions solidity/test/unit/modules/dispute/BondEscalationAccounting.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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);
Expand All @@ -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,
Expand Down
Loading