UniswapHandler cache storage array length

[code423n4]

Handle

GiveMeTestEther

Vulnerability details

Impact

Cache the storage array length read (incl. subtraction of 1) to save the (warm) storage read and the subtraction each loop iteration.

https://eips.ethereum.org/EIPS/eip-2929 states WARM_STORAGE_READ_COST = 100 gas
creating a local variable to store the storage array length will be way cheaper because it is stored on the stack (PUSH 3 gas (store) + POP 2 GAS (read) + other cheap ops)

Note: on https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L304
there is already an access of the array length, so it would also save gas to already store the array length before the if statement in a local variable, because the assumption is that this function will almost only be called if the array is not empty. And after the if statement decrement the variable by 1.

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/c3a204a2c0f7c653c6c2dda9f4563fd1dc1cecf3/src/contracts/DexHandlers/UniswapHandler.sol#L315

Tools Used

Manual Analysis

Recommended Mitigation Steps

• replace to both occurrence of buyers.length - 1 as follows:
  -uint buyersLengthMinusOne = buyers.length - 1;

  for (uint i = 0; i < buyersLengthMinusOne; i = i + 1) {
  if (buyers[i] == _buyer) {
  // Replace the current item with the last and pop the last away.
  buyers[i] = buyers[buyersLengthMinusOne];
  buyers.pop();
  return;
  }
  }

[0xScotch]

#106