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

Jump to bottom

fix: adding escalated status check #46

Merged
merged 8 commits into from
Jul 18, 2024
Merged

fix: adding escalated status check #46

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
55afd31
fix: adding escalated status
ashitakah Jul 8, 2024
6e639d5
Merge branch 'dev' into fix/dispute-lock
0xShaito Jul 9, 2024
f1009d0
Merge branch 'dev' of github.com:defi-wonderland/prophet-modules into…
ashitakah Jul 13, 2024
bd1378e
fix: comments
ashitakah Jul 13, 2024
ec34e68
Merge branch 'fix/dispute-lock' of github.com:defi-wonderland/prophet…
ashitakah Jul 13, 2024
7100164
Merge branch 'dev' of github.com:defi-wonderland/prophet-modules into…
ashitakah Jul 17, 2024
cdecadb
feat: tests
ashitakah Jul 17, 2024
52ceaab
fix: comments
ashitakah Jul 17, 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
10 changes: 8 additions & 2 deletions solidity/contracts/modules/resolution/PrivateERC20ResolutionModule.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
function commitVote(IOracle.Request calldata _request, IOracle.Dispute calldata _dispute, bytes32 _commitment) public {
bytes32 _disputeId = _getId(_dispute);
if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute();
if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None) {
if (
0xShaito marked this conversation as resolved.
Show resolved Hide resolved
ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None
&& ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated
Copy link
Member

Choose a reason for hiding this comment

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

The finding only says to check against IOracle.DisputeStatus.Escalated. Does it also need to keep None? What is that 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.

Yep we should left the None also, is the other case which the dispute can be voted

Copy link
Member

Choose a reason for hiding this comment

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

@0xShaito @0xJabberwock the None status means the dispute has not started yet.

There are 2 somewhat realistic scenarios that could lead to a dispute having the None in this function:

  1. The ID is incorrect, in which case ORACLE.createdAt(_disputeId) == 0 effectively duplicates this check.
  2. The dispute was created in a messed up module that somehow called the updateDisputeStatus function of the oracle and set an invalid status.

The second point is a bit concerning but the off-chain logic should catch that, so I would remove the None check.

) {
revert PrivateERC20ResolutionModule_AlreadyResolved();
}

Expand Down Expand Up @@ -114,7 +117,10 @@ contract PrivateERC20ResolutionModule is Module, IPrivateERC20ResolutionModule {
IOracle.Dispute calldata _dispute
) external onlyOracle {
if (ORACLE.createdAt(_disputeId) == 0) revert PrivateERC20ResolutionModule_NonExistentDispute();
if (ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None) {
if (
ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.None
&& ORACLE.disputeStatus(_disputeId) != IOracle.DisputeStatus.Escalated
0xJabberwock marked this conversation as resolved.
Show resolved Hide resolved
) {
revert PrivateERC20ResolutionModule_AlreadyResolved();
0xJabberwock marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
62 changes: 62 additions & 0 deletions solidity/test/unit/modules/resolution/PrivateERC20ResolutionModule.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,68 @@ contract PrivateERC20ResolutionModule_Unit_CommitVote is BaseTest {
module.commitVote(mockRequest, mockDispute, _commitment);
}

/**
* @notice Test that `commitVote` reverts if called with `_disputeId` of an already active dispute.
*/
function test_revertIfActive(bytes32 _requestId, bytes32 _commitment) public {
// Computer proper IDs
mockDispute.requestId = _requestId;
bytes32 _disputeId = _getId(mockDispute);

// Mock and expect IOracle.createdAt to be called
_mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1));
// Mock and expect IOracle.disputeStatus to be called
_mockAndExpect(
address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Active)
);

// Check: does it revert if the dispute is already resolved?
vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector);
module.commitVote(mockRequest, mockDispute, _commitment);
}

/**
* @notice Test that `commitVote` reverts if called with `_disputeId` of a dispute with no resolution.
*/
function test_revertIfNoResolution(bytes32 _requestId, bytes32 _commitment) public {
// Computer proper IDs
mockDispute.requestId = _requestId;
bytes32 _disputeId = _getId(mockDispute);

// Mock and expect IOracle.createdAt to be called
_mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1));
// Mock and expect IOracle.disputeStatus to be called
_mockAndExpect(
address(oracle),
abi.encodeCall(IOracle.disputeStatus, (_disputeId)),
abi.encode(IOracle.DisputeStatus.NoResolution)
);

// Check: does it revert if the dispute is already resolved?
vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector);
module.commitVote(mockRequest, mockDispute, _commitment);
}

/**
* @notice Test that `commitVote` reverts if called with `_disputeId` of a dispute that has already been won.
*/
function test_revertIfWon(bytes32 _requestId, bytes32 _commitment) public {
// Computer proper IDs
mockDispute.requestId = _requestId;
bytes32 _disputeId = _getId(mockDispute);

// Mock and expect IOracle.createdAt to be called
_mockAndExpect(address(oracle), abi.encodeCall(IOracle.createdAt, (_disputeId)), abi.encode(1));
// Mock and expect IOracle.disputeStatus to be called
_mockAndExpect(
address(oracle), abi.encodeCall(IOracle.disputeStatus, (_disputeId)), abi.encode(IOracle.DisputeStatus.Won)
);

// Check: does it revert if the dispute is already resolved?
vm.expectRevert(IPrivateERC20ResolutionModule.PrivateERC20ResolutionModule_AlreadyResolved.selector);
module.commitVote(mockRequest, mockDispute, _commitment);
}

/**
* @notice Test that `commitVote` reverts if called with `_disputeId` of an already resolved dispute.
*/
Expand Down
Loading