Sherlock contest - bobi's results

Handle

bobi

Vulnerability details

Impact

unchecked: Using the unchecked keyword to avoid redundant arithmetic underflow/overflow checks to save gas when an underflow/overflow cannot happen.

Proof of Concept

As an example, on can add unchecked on a for loop's iterator( from contracts/managers/Manager.sol:46) as so:

for (uint256 i; i < _extraTokens.length; i++) {
...
}

Into a more efficient:

for (uint256 i = 0; i < _extraTokens.length; unchecked{++i}) {
...
}

Since the uint256's max cannot be reached, it is safe to use.

Handle

bobi

Vulnerability details

Impact

.length is called on each for indexing, causing extra gas to be used

Proof of Concept

storage > memory >= calldata reads > local variable reads.

For example, in the SherBuy.sol you have:

for (uint256 i; i < _tokens.length; i++) {
   IERC20 token = _tokens[I];
   ...

On each iteration, the .length will be called; to fix this, you could cache it in a local variable as so: (this applies wherever in the codebase possible)

uint256 _length = _tokens.length;
for (uint256 i; i < _length; i++) {
   IERC20 token = _tokens[I];
   ...

Tools Used

Manual analysis.

Recommended Mitigation Steps

Cache the .length of the arrays.

Handle

bobi

Vulnerability details

Impact

There are some function calls that can be done after subsequent checks in order to avoid unnecessary calls, save gas, and keep a consistent coding style.

Proof of Concept

For example, this part of AaveV2Strategy.sol:

function withdrawAll() external override onlySherlockCore returns (uint256) {
    ILendingPool lp = getLp();
    if (balanceOf() == 0) {
      return 0;
    }
   ...

The internal call of getLp() can be called after the if() check. This pattern appears on:

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/AaveV2Strategy.sol#L81-L83

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/AaveV2Strategy.sol#L63-L67

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L657-L668

https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/Sherlock.sol#L408-L417

Recommended Mitigation Steps

Use a similar pattern(check - first, call - after) to the one at: https://github.com/code-423n4/2022-01-sherlock/blob/main/contracts/managers/AaveV2Strategy.sol#L92-L96

function withdraw(uint256 _amount) external override onlySherlockCore {
  // Why do we only check if _amount is equal to the max value?
  if (_amount == type(uint256).max) revert InvalidArgument();

  ILendingPool lp = getLp();