Unnecessary checked arithmetic in for-loops

[code423n4]

Handle

Dravee

Vulnerability details

Impact

In Solidity 0.8+, there's a default overflow check on unsigned integers. It's possible to uncheck this in for-loops and save 77 gas per iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

ethereum/solidity#10695

Proof of Concept

Instances include:

./NFTXEligibilityManager.sol:85:        for (uint256 i = 0; i < modulesCopy.length; i++) {
./NFTXLPStaking.sol:81:        for (uint256 i = 0; i < vaultIds.length; i++) {
./NFTXLPStaking.sol:206:        for (uint256 i = 0; i < vaultIds.length; i++) {
./NFTXMarketplaceZap.sol:263:    for (uint256 i = 0; i < idsIn.length; i++) {
./NFTXMarketplaceZap.sol:297:    for (uint256 i = 0; i < idsIn.length; i++) {
./NFTXMarketplaceZap.sol:379:    for (uint256 i = 0; i < ids.length; i++) {
./NFTXMarketplaceZap.sol:399:    for (uint256 i = 0; i < ids.length; i++) {
./NFTXMarketplaceZap.sol:414:    for (uint256 i = 0; i < ids.length; i++) {
./NFTXMarketplaceZap.sol:437:    for (uint256 i = 0; i < idsIn.length; i++) {
./NFTXSimpleFeeDistributor.sol:62:    for (uint256 i = 0; i < length; i++) {
 {
./NFTXVaultUpgradeable.sol:364:        for (uint256 i = 0; i < len; i++) {
./NFTXVaultUpgradeable.sol:406:            for (uint256 i = 0; i < tokenIds.length; i++) {
./NFTXVaultUpgradeable.sol:419:            for (uint256 i = 0; i < tokenIds.length; i++) {
./NFTXVaultUpgradeable.sol:442:        for (uint256 i = 0; i < amount; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

The code would go from:

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

to:

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

While the risk are overflow is inexistant for a uint256 i, you might want to manually check for bounds once before the for-loop if i is smaller than a uint256 (such as uint8, never recommended and not the case in this project).