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

fix: resolution modules can only update the dispute's status forward #69

Open
wants to merge 1 commit into
base: fix/oz-audit
Choose a base branch
from
Open

fix: resolution modules can only update the dispute's status forward #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions solidity/contracts/Oracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,11 @@ contract Oracle is IOracle, OracleAccessController {
if (msg.sender != _request.disputeModule && msg.sender != _request.resolutionModule) {
revert Oracle_NotDisputeOrResolutionModule(msg.sender);
}

if (msg.sender == _request.resolutionModule && _status <= DisputeStatus.Escalated) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Its true that <= is a very good solution to optimize, but what if we add new dispute status. IDK , for now is fine I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before adding a new state, we'd have to ensure the change is fully compatible with all existing modules and their respective flows. This will definitely be a pain point in future developments.

In this PR I specifically adhere to what the auditor found: a poorly implemented module could reverse the state of a dispute to a previous state, which is unexpected and could break an invariant (is it really an invariant? I don't think we have defined them yet)

revert Oracle_InvalidStatusUpdate();
}

disputeStatus[_disputeId] = _status;
IDisputeModule(_request.disputeModule).onDisputeStatusChange(_disputeId, _request, _response, _dispute);

Expand Down
5 changes: 5 additions & 0 deletions solidity/interfaces/IOracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,11 @@ interface IOracle is IOracleAccessController {
*/
error Oracle_InvalidDisputer();

/**
* @notice Thrown when a resolution module updates the status of a dispute to an invalid status
*/
error Oracle_InvalidStatusUpdate();

/*///////////////////////////////////////////////////////////////
ENUMS
//////////////////////////////////////////////////////////////*/
Expand Down
22 changes: 22 additions & 0 deletions solidity/test/unit/Oracle.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,28 @@ contract Oracle_Unit_UpdateDisputeStatus is BaseTest {
/**
* @notice If the dispute does not exist, the call should revert
*/
function test_updateDisputeStatus_revertsIfResolutionStatusIsInvalid(uint256 _newStatus) public {
// 0 to 3 status, from status None to Escalated.
_newStatus = bound(_newStatus, 0, 2);

bytes32 _disputeId = _getId(mockDispute);
bytes32 _responseId = _getId(mockResponse);

// Mock the dispute
oracle.mock_setDisputeOf(_responseId, _disputeId);
oracle.mock_setDisputeCreatedAt(_disputeId, block.timestamp);

// Check: revert?
vm.expectRevert(IOracle.Oracle_InvalidStatusUpdate.selector);

// Test: resolution module updates to an invalid status
vm.prank(address(resolutionModule));
oracle.updateDisputeStatus(mockRequest, mockResponse, mockDispute, IOracle.DisputeStatus(_newStatus));
}

/**
* @notice If a resolution modules tries to update the dispute's status to a status that is not allowed, the call should revert
*/
function test_updateDisputeStatus_revertsIfInvalidDispute() public {
bytes32 _disputeId = _getId(mockDispute);

Expand Down
Loading