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

&& operator can use more gas #128

Open
code423n4 opened this issue Jan 6, 2022 · 3 comments
Open

&& operator can use more gas #128

code423n4 opened this issue Jan 6, 2022 · 3 comments
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) 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

Handle

rfa

Vulnerability details

Impact

more expensive gas usage

Proof of Concept

instead of using operator && on single require check (XDEFIDistribution.sol line 255). using double require check can save more gas:

require(amount_ != uint256(0) && amount_ <= MAX_TOTAL_XDEFI_SUPPLY, "INVALID_AMOUNT");

Tools Used

Recommended Mitigation Steps

require(amount_ != uint256(0), "INVALID_AMOUNT" );
require(amount_ <= MAX_TOTAL_XDEFI_SUPPLY, "INVALID_AMOUNT");

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Jan 6, 2022
code423n4 added a commit that referenced this issue Jan 6, 2022
@deluca-mike deluca-mike added the invalid This doesn't seem right label Jan 8, 2022
@deluca-mike
Copy link
Collaborator

While two seperate require checks result in about 14 gas saved on lock, relock, and relockBatch, it costs 10923 more gas to deploy, which it worth it after 780 calls. I guess this is a fair trade-off.
In any case, due to the move to if-reverts and custom error messages, and the removal of the MAX_TOTAL_XDEFI_SUPPLY variable, and it's check, this is being changed to a check on the resulting computed units:

if (units == uint256(0)) revert LockResultsInNoUnits();

@deluca-mike deluca-mike added sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed invalid This doesn't seem right labels Jan 8, 2022
@deluca-mike
Copy link
Collaborator

In release candidate contracts, amount_ is no longer checked, and thus MAX_TOTAL_XDEFI_SUPPLY is not needed, and the condition is no longer two-fold. Instead, resulting units are checked to be greater or equal to some minimum.

@deluca-mike deluca-mike added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Jan 14, 2022
@Ivshti
Copy link
Member

Ivshti commented Jan 16, 2022

resolved & confirmed

@Ivshti Ivshti closed this as completed Jan 16, 2022
@CloudEllie CloudEllie reopened this Jan 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) 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

4 participants