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

setLinkToken-Is-Publicly-Exposed #12504

Merged
merged 3 commits into from
Mar 20, 2024
Merged

setLinkToken-Is-Publicly-Exposed #12504

merged 3 commits into from
Mar 20, 2024

Conversation

leeyikjiun
Copy link
Contributor

  • Remove setLinkToken in VRFV2PlusWrapperConsumerBase and made link immutable. Link is now retrieved from VRFV2PlusWrapper on constructor.

  • Made link immutable in VRFV2PlusWrapper and check if address is non zero on constructor.

@leeyikjiun leeyikjiun requested review from a team as code owners March 20, 2024 06:31
@github-actions GitHub Actions
Copy link
Contributor

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset.

@github-actions GitHub Actions
Copy link
Contributor

I see you updated files related to contracts. Please run pnpm changeset in the contracts directory to add a changeset.

@leeyikjiun leeyikjiun requested a review from a team as a code owner March 20, 2024 07:09
@leeyikjiun leeyikjiun force-pushed the VRF-950-Audit-Fix-SP-5-setLinkToken-Is-Publicly-Exposed branch from e0a0925 to f26a274 Compare March 20, 2024 07:15
@leeyikjiun leeyikjiun force-pushed the VRF-950-Audit-Fix-SP-5-setLinkToken-Is-Publicly-Exposed branch from f26a274 to 3cf916f Compare March 20, 2024 07:43
ibrajer
ibrajer reviewed Mar 20, 2024
View reviewed changes
contracts/test/v0.8/foundry/vrf/VRFV2PlusWrapper.t.sol
function testSetLinkAndLinkNativeFeed() public {
VRFV2PlusWrapper wrapper = new VRFV2PlusWrapper(address(0), address(0), address(s_testCoordinator));
function testSetLinkNativeFeed() public {
VRFV2PlusWrapper wrapper = new VRFV2PlusWrapper(address(s_linkToken), address(0), address(s_testCoordinator));
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: add a check that the constructor will revert with ZeroAddress if the LINK token address is set to zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. I wanted to add that test but I forgot about it. Thanks for catching this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it's sort of already tested through this PR

ibrajer
ibrajer reviewed Mar 20, 2024
View reviewed changes
contracts/src/v0.8/vrf/dev/interfaces/IVRFV2PlusWrapper.sol Outdated
@@ -87,4 +87,6 @@ interface IVRFV2PlusWrapper {
uint32 _numWords,
bytes memory extraArgs
) external payable returns (uint256 requestId);

function link() external returns (address);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this not a view function?

@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

ibrajer
ibrajer approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@ibrajer ibrajer left a comment

Choose a reason for hiding this comment

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

LGTM

@leeyikjiun leeyikjiun added this pull request to the merge queue Mar 20, 2024
Merged via the queue into develop with commit 3eba7a2 Mar 20, 2024
115 of 116 checks passed
@leeyikjiun leeyikjiun deleted the VRF-950-Audit-Fix-SP-5-setLinkToken-Is-Publicly-Exposed branch March 20, 2024 14:03
@RensR RensR changed the title VRF-950-Fix-SP-5-setLinkToken-Is-Publicly-Exposed setLinkToken-Is-Publicly-Exposed Mar 26, 2024
kidambisrinivas pushed a commit that referenced this pull request Mar 27, 2024
@leeyikjiun @kidambisrinivas
VRF-950-Fix-SP-5-setLinkToken-Is-Publicly-Exposed (#12504)
d7b9e9d 
* Remove setLinToken from VRFV2PlusWrapperConsumerBase

* Add missing view to IVRFV2PlusWrapper link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibrajer ibrajer ibrajer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@leeyikjiun @ibrajer