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.

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

GovNFT: maxBridge has no effect #334

Open
code423n4 opened this issue Dec 16, 2022 · 9 comments
Open

GovNFT: maxBridge has no effect #334

code423n4 opened this issue Dec 16, 2022 · 9 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working judge review requested Judge should review this issue M-08 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L19-L20

Vulnerability details

Impact

In GovNFT, setMaxBridge function is provided to set maxBridge, but this variable is not used, literally it should be used to limit the number of GovNFTs crossing chain, but it doesn't work in GovNFT.

    uint256 public maxBridge = 20;
...
    function setMaxBridge(uint256 _max) external onlyOwner {
        maxBridge = _max;
    }

Proof of Concept

https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L19-L20
https://github.com/code-423n4/2022-12-tigris/blob/496e1974ee3838be8759e7b4096dbee1b8795593/contracts/GovNFT.sol#L311-L313

Tools Used

None

Recommended Mitigation Steps

Consider applying the maxBridge variable

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 16, 2022
code423n4 added a commit that referenced this issue Dec 16, 2022
@GalloDaSballo
Copy link

R

@c4-judge
Copy link
Contributor

GalloDaSballo changed the severity to QA (Quality Assurance)

@c4-judge c4-judge added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax downgraded by judge Judge downgraded the risk level of this issue and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Dec 20, 2022
@GainsGoblin
Copy link

I disagree with the severity downgrade.

@c4-sponsor
Copy link

GainsGoblin requested judge review

@GalloDaSballo
Copy link

@GainsGoblin Thank you for flagging.

I have checked the LayerZero Library and it seems like it will not revert when relaying a tx that is too expensive.

For this reason I agree with you and will raise Severity to Medium.

The Warden has shown how, an unused variable, which was meant to cap the amount of tokens bridged per call, could cause a DOS.

These types of DOS could only be fixed via Governance Operations, and could create further issues, for this reason I agree with Medium Seveirty

@Simon-Busch Simon-Busch added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value primary issue Highest quality submission among a set of duplicates and removed downgraded by judge Judge downgraded the risk level of this issue QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jan 23, 2023
@Simon-Busch
Copy link

Reverted back to M and set as Primary issue as requested by @GalloDaSballo

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Jan 23, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as selected for report

@GainsGoblin
Copy link

GainsGoblin commented Jan 29, 2023

Mitigation: code-423n4/2022-12-tigris#2 (comment)

@c4-sponsor
Copy link

GainsGoblin marked the issue as sponsor confirmed

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working judge review requested Judge should review this issue M-08 primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

7 participants