code-423n4 / 2022-01-xdefi-findings

0 stars 0 forks source link

Unnecessary checked arithmetic in for loops #107

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for for (uint256 i; i < count; ++i)).

Increments perform overflow checks that are not necessary in this case.

Recommendation

Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the for loop:

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

for (uint256 i; i < count; ++i) {
    uint256 duration = durations_[i];
    require(duration <= uint256(18250 days), "INVALID_DURATION");
    emit LockPeriodSet(duration, bonusMultiplierOf[duration] = multipliers[i]);
}

to:

for (uint256 i; i < count;) {
    uint256 duration = durations_[i];
    require(duration <= uint256(18250 days), "INVALID_DURATION");
    emit LockPeriodSet(duration, bonusMultiplierOf[duration] = multipliers[i]);

    unchecked { ++i; }
}
deluca-mike commented 2 years ago

Agreed. We originally abstained from unchecked math, for cleaner code, but we will now use unchecked wherever possible.

deluca-mike commented 2 years ago

Duplicate #9