feat: access control #52
Conversation
|
|
||
| /// @inheritdoc IOracle | ||
| function finalize(IOracle.Request calldata _request, IOracle.Response calldata _response) external { | ||
| function finalize( |
There was a problem hiding this comment.
Why do we need the msg.senders in finalize? Is there a use for it? If not sure, can you ask in the prophet channel what the intended use is?
There was a problem hiding this comment.
commented in the discord channel
There was a problem hiding this comment.
It would be meaningful to revise what each existing implementation of the Prophet modules expects as to the _finalizer parameter of finalizeRequest().
- Apparently, all dispute and resolution modules don't utilize it.
- Moreover, all request and finality modules only do it upon the emission of
RequestFinalizedevent (except forEBORequestModule). - However, the unique response module does so with the aim of identifying whether the argument is an
Oracle::allowedModule, apart from for the event emission. - Lastly, mind that
CircuitResolverModuleandRootVerificationModule(both dispute modules) and EBO'sCouncilArbitrator(not a module) callOracle::finalize.
|
|
||
| /// @inheritdoc IOracle | ||
| function finalize(IOracle.Request calldata _request, IOracle.Response calldata _response) external { | ||
| function finalize( |
There was a problem hiding this comment.
It would be meaningful to revise what each existing implementation of the Prophet modules expects as to the _finalizer parameter of finalizeRequest().
- Apparently, all dispute and resolution modules don't utilize it.
- Moreover, all request and finality modules only do it upon the emission of
RequestFinalizedevent (except forEBORequestModule). - However, the unique response module does so with the aim of identifying whether the argument is an
Oracle::allowedModule, apart from for the event emission. - Lastly, mind that
CircuitResolverModuleandRootVerificationModule(both dispute modules) and EBO'sCouncilArbitrator(not a module) callOracle::finalize.
solidity/contracts/Oracle.sol
Outdated
| // The caller must be the proposer, unless the response is coming from a dispute module | ||
| if (msg.sender != _response.proposer && msg.sender != address(_request.disputeModule)) { | ||
| if (_accessControl.user != _response.proposer && _accessControl.user != address(_request.disputeModule)) { |
There was a problem hiding this comment.
If I correctly understand the new intended behaviour:
- the user, not the caller, must be the proposer,
- even though the response may come from a dispute module (i.e.,
CircuitResolverModuleorRootVerificationModule).
There was a problem hiding this comment.
Such dispute modules may set themselves as the access control users but not as the proposers; considering that that is the new expected behaviour:
- The user, not the caller, must be the proposer,
- unless the response is coming from a dispute module (i.e.,
CircuitResolverModuleorRootVerificationModule).
The code comment should be updated accordingly.
solidity/contracts/Oracle.sol
Outdated
| external | ||
| isApproved(_accessControl.user, _request.accessControlModule) | ||
| hasAccess(_request.accessControlModule, PROPOSE_TYPEHASH, abi.encode(_request, _response), _accessControl) | ||
| returns (bytes32 _responseId) | ||
| { | ||
| _responseId = ValidatorLib._validateResponse(_request, _response); |
There was a problem hiding this comment.
Reminder: executing isApproved and hasAccess modifiers before any other validation might pose a security vulnerability, given that Oracle would perform an external call to an arbitrary address with arbitrary data from the user.
solidity/contracts/Oracle.sol
Outdated
| // Needed to avoid stack too deep error | ||
| Request memory _usedRequest = _request; | ||
| Response memory _usedResponse = _response; | ||
| Dispute memory _usedDispute = _dispute; | ||
|
|
There was a problem hiding this comment.
How come defining even more variables fixes the problem?
There was a problem hiding this comment.
I'll create a task to track this, but probably we shouldn't merge this PR without fixing it.
| @@ -94,10 +101,13 @@ contract BaseTest is Test, Helpers { | |||
| event RequestCreated(bytes32 indexed _requestId, IOracle.Request _request, bytes32 _ipfsHash); | |||
| event ResponseProposed(bytes32 indexed _requestId, bytes32 indexed _responseId, IOracle.Response _response); | |||
| event ResponseDisputed(bytes32 indexed _responseId, bytes32 indexed _disputeId, IOracle.Dispute _dispute); | |||
| event OracleRequestFinalized(bytes32 indexed _requestId, bytes32 indexed _responseId, address indexed _caller); | |||
| event OracleRequestFinalized(bytes32 indexed _requestId, bytes32 indexed _responseId); | |||
There was a problem hiding this comment.
This is not blocking the PR, but FYI we can import events from IOracle instead of defining them in unit tests. For example:
vm.expectEmit(address(_staking));
emit IStaking.Staked(_user, _fuzz.amount);Note that the emit call includes the full "path" to the event, starting from the interface name. This allows to pull events from anywhere really, reducing the duplication and as well avoiding issues with mismatching declarations of events in interfaces and tests.
There was a problem hiding this comment.
Very nice! Tracked in OPT-536
Closes OPT-493