Conversation
Pull Request Test Coverage Report for Build 22578413876Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR fixes the updateRequest function in Verifier.sol, where the stored creator field was incorrectly being overwritten with _msgSender() (the caller, i.e., the contract owner) instead of using the creator address supplied in the request struct. A version bump from 3.4.0 to 3.4.1 is also included.
Changes:
Verifier.sol: In_updateRequest, changecreator: _msgSender()tocreator: request.creatorso the creator field is taken from the request calldata.universal-verifier.test.ts: Add assertions verifying thatrequestStored.creatoris correctly set both before and after anupdateRequestcall.package.json/package-lock.json: Version bumped to3.4.1.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
contracts/verifiers/Verifier.sol |
Core fix: _updateRequest now stores request.creator from calldata instead of _msgSender() |
test/verifier/universal-verifier.test.ts |
Test assertions added to verify the creator field before and after updateRequest |
contracts/package.json |
Version bumped from 3.4.0 to 3.4.1 |
contracts/package-lock.json |
Version aligned with package.json at 3.4.1 (was previously out of sync at 3.3.0) |
Issues found:
-
Critical bug — state inconsistency between
RequestData.creatorand_requestOwners: The_updateRequestfunction updatesRequestData.creatorin the_requestsmapping, but it does not update the_requestOwnersmapping maintained byRequestOwnership. After callingupdateRequest,getRequest(requestId).creatorandgetRequestOwner(requestId)will return different addresses. SinceonlyOwnerOrRequestOwner(used bysetRequestOwner,disableRequest,enableRequest) relies ongetRequestOwner(), the original request owner can still exercise those privileges even after the storedcreatorfield has been changed. -
Security concern: Using
request.creatorfrom calldata allows the contract owner to set thecreatorfield to any arbitrary address, includingaddress(0). This is inconsistent with_setRequest, which always stores_msgSender()— it may be intentional, but it should be validated. -
Missing test coverage: The new test does not call
getRequestOwner()after the update, so the above divergence betweencreatorand_requestOwnersgoes undetected.
Files not reviewed (1)
- contracts/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| validator: request.validator, | ||
| params: request.params, | ||
| creator: _msgSender() | ||
| creator: request.creator |
There was a problem hiding this comment.
The _updateRequest function stores request.creator directly from caller-supplied calldata, allowing the contract owner to set the creator field to any arbitrary address (including address(0) or unrelated accounts). In contrast, _setRequest always stores _msgSender() as the creator. If the intent is for updateRequest to preserve the original creator, the stored value from s._requests[request.requestId].creator should be used instead. If the intent is to allow the owner to change the creator, consider whether setting it to address(0) should be prevented, and whether the RequestOwnership._requestOwners mapping should also be updated accordingly.
| function _updateRequest( | ||
| IVerifier.Request calldata request | ||
| ) internal checkRequestExistence(request.requestId, true) { | ||
| VerifierStorage storage s = _getVerifierStorage(); | ||
|
|
||
| s._requests[request.requestId] = IVerifier.RequestData({ | ||
| metadata: request.metadata, | ||
| validator: request.validator, | ||
| params: request.params, | ||
| creator: _msgSender() | ||
| creator: request.creator | ||
| }); | ||
| } |
There was a problem hiding this comment.
When _updateRequest changes RequestData.creator, the _requestOwners mapping in RequestOwnership is NOT updated. This creates a state inconsistency: after calling updateRequest, getRequest(requestId).creator and getRequestOwner(requestId) can return different addresses. Since the onlyOwnerOrRequestOwner modifier (used by setRequestOwner, disableRequest, enableRequest) relies on getRequestOwner(), this inconsistency can cause confusing access control outcomes — the original request owner may still be able to manage the request even after the stored creator has changed.
The test at line 490 asserts requestStored.creator == owner.address after an update, but does not verify that getRequestOwner() is also updated, leaving this divergence untested.
| requestStored = await verifier.getRequest(requestId); | ||
| expect(requestStored.metadata).to.be.equal("metadata2"); | ||
| expect(requestStored.creator).to.be.equal(owner.address); |
There was a problem hiding this comment.
The "Check updateRequest" test does not verify that getRequestOwner(requestId) reflects the new creator after calling updateRequest. Since _requestOwners (used by getRequestOwner) is not updated by _updateRequest, getRequestOwner() will continue returning the original request owner (requestOwner.address) even though getRequest().creator now returns owner.address. A test case asserting the expected value of getRequestOwner() after the update would catch this divergence.
No description provided.