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

_zeroDurationPointBase can potentially be exploited to get more scores #139

Open
code423n4 opened this issue Jan 6, 2022 · 2 comments
Open
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working 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

WatchPug

Vulnerability details

_zeroDurationPointBase can be set at deploy time so that locks with 0 duration can get scores.

However, if the value of _zeroDurationPointBase is being set high enough. It can potentially be exploited by repeatedly lock(), and unlock() with 0 duration to get scores.

This can get amplified with flashloans.

https://github.com/XDeFi-tech/xdefi-distribution/blob/3856a42df295183b40c6eee89307308f196612fe/contracts/XDEFIDistribution.sol#L245-L247

function _getPoints(uint256 amount_, uint256 duration_) internal view returns (uint256 points_) {
    return amount_ * (duration_ + _zeroDurationPointBase);
}

Recommendation

Consider changing _zeroDurationPointBase to a constant of value 1.

@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Jan 6, 2022
code423n4 added a commit that referenced this issue Jan 6, 2022
@deluca-mike deluca-mike added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") and removed disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) labels Jan 8, 2022
@deluca-mike
Copy link
Collaborator

deluca-mike commented Jan 8, 2022

I thought about it some more and I have to agree that if we did allow a 0 duration, this entire score system is unenforceable due to flash loans. So, because of that, I'm going to make the score function simply amount_ * duration_, and remove _zeroDurationPointBase from the contract. If we wanted "mimial" lock duration, we can simply do something small like 1 day, or even 1 second. And if we did allow 0 seconds, then it's only fair that "flash loaner" gets an NFT of 0 score.

@deluca-mike
Copy link
Collaborator

In the release candidate contract, _zeroDurationPointBase has been removed, and it is no longer possible to lock for 0 duration, since enabling such a duration via setLockPeriods is prevented.

@deluca-mike deluca-mike added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label Jan 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working 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

2 participants