code-423n4 / 2021-11-badgerzaps-findings

0 stars 0 forks source link

Unnecessary `SLOAD`s / `MLOAD`s / `CALLDATALOAD`s in for-each loops #58

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

pants

Vulnerability details

Vulnerability details

At Zap.sol init function you have the for loop pattern:

for (uint256 i = 0; i < array.length; i++) { // do something with array[i] }


In such for loops, the `array.length` is read on every iteration, instead of caching it once in a local variable and read it again using the local variable.

## Impact
Storage reads are much more expensive than reading local variables. Memory and calldata reads are a bit more expensive than reading local variables.

## Tool Used
Manual code review.

## Recommended Mitigation Steps
Read these values from storage / memory / calldata once, cache them in local variables and then read them again using the local variables. For example:

uint256 length = array.length; for (uint256 i = 0; i < length; i++) { // do something with array[i] }

GalloDaSballo commented 3 years ago

Agree we should have cached the length value