Unnecessary checked arithmetic in for loops

[code423n4]

Handle

pants

Vulnerability details

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

Impact

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

Tool Used

Manual code review.

Recommended Mitigation Steps

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

for (uint256 i = 0; i < numIterations; i++) {
    // ...
}

to

for (uint256 i = 0; i < numIterations;) {
    // ...
    unchecked { i++; }
}

It is a little less readable but it saves a significant amount of gas.

[Shadowfiend]

The only for loop in Swap is in fee sweeping (https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#L250-L256). Since the tokens list is passed externally and the iteration variable is a uint8 with max value 255, I believe there is in fact risk of overflow (see also #80 ).

[mhluongo]

We could check the bounds once before the loop rather than checking it each iteration though.

[Shadowfiend]

Yeah there are a few other issues that cover that kind of thing under the auspices of gas optimization.

[0xean]

This is a valid optimization especially since the uint8 should be a uint256 based on other wardens comments. Leaving as valid and open