Nested Finance contest - PierrickGT's results

BeefyVaultOperator.sol

deposit: setMaxAllowance should be called in the constructor

In the deposit function on line 48, we call ExchangeHelpers.setMaxAllowance(token, vault); to allow the vault to spend token.

Each time assets are deposited in the vault, we shouldn't have to allow it to spend the token again. I recommend to call ExchangeHelpers.setMaxAllowance(token, vault) only once in the constructor for each vault and token. I also recommend to add a setMaxAllowance function only callable by the owner of the operator that would allow to set the max allowance in case the allowance has decreased.

Recommendation:

for (uint256 i; i < vaultsLength; i++) {
    operatorStorage.addVault(vaults[i], tokens[i]);
    ExchangeHelpers.setMaxAllowance(tokens[i], vaults[i]);
}

BeefyZapBiswapLPVaultOperator.sol and BeefyZapUniswapLPVaultOperator.sol

zapAndStakeLp: setMaxAllowance should be called in the constructor

In the _zapAndStakeLp function on line 189 and subsequently on line 194 and 195 we call ExchangeHelpers.setMaxAllowance(); to allow the vault to spend token.

Each time assets are deposited in the vault, we shouldn't have to allow it to spend the token again. I recommend to call ExchangeHelpers.setMaxAllowance() only once in the constructor for each vault and token. I also recommend to add a setMaxAllowance function only callable by the owner of the operator that would allow to set the max allowance in case the allowance has decreased.

This will also avoid calling ExchangeHelpers.setMaxAllowance(IERC20(swapToken), router); in the _withdrawAndSwap function on line 162.

Recommendation:

for (uint256 i; i < vaultsLength; i++) {
    operatorStorage.addVault(vaults[i], routers[i]);

    IBiswapPair pair = IBiswapPair(IBeefyVaultV6(vaults[i]).want());

    ExchangeHelpers.setMaxAllowance(IERC20(address(pair)), address(vaults[i]));
    ExchangeHelpers.setMaxAllowance(IERC20(pair.token0()), routers[i]);
    ExchangeHelpers.setMaxAllowance(IERC20(pair.token1()), routers[i]);
}

YearnCurveVaultOperator.sol

depositETH: setMaxAllowance should be called in the constructor

In the depositETH function on line 78, we call ExchangeHelpers.setMaxAllowance(IERC20(address(weth)), address(withdrawer)); to allow the withdrawer to spend weth.

Each time assets are deposited in the vault, we shouldn't have to allow the withdrawer to spend weth again. I recommend to call ExchangeHelpers.setMaxAllowance(IERC20(address(weth)), address(withdrawer)) only once in the constructor. I also recommend to add a setMaxAllowance function only callable by the owner of the operator that would allow to set the max allowance in case the allowance has decreased.

Recommendation:

ExchangeHelpers.setMaxAllowance(IERC20(_weth), address(_withdrawer));

StakingLPVaultHelpers.sol

addLiquidityAndDepositETH: setMaxAllowance should be called in addVault

In the addLiquidityAndDepositETH function on line 45, we call ExchangeHelpers.setMaxAllowance(lpToken, vault); to allow the vault to spend lpToken.

Each time assets are deposited in the vault, we shouldn't have to allow the vault to spend lpToken again. I recommend to call ExchangeHelpers.setMaxAllowance(lpToken, vault); only once in the addVault function of the YearnVaultStorage. I also recommend to add a setMaxAllowance function only callable by the owner that would allow to set the max allowance in case the allowance has decreased.

This will also avoid calling it in the _addLiquidityAndDeposit function on line 77

Recommendation:

ExchangeHelpers.setMaxAllowance(curvePool.lpToken, vault);

NestedFactory.sol

transferToReserveAndStore: balanceReserveAfter can be inlined

In the _transferToReserveAndStore function, we store the reserve balance after the transfer in the balanceReserveAfter variable, on line 523.

This variable being only used once, we can inline it and save one mstore.

Recommendation:

nestedRecords.store(_nftId, address(_token), _token.balanceOf(reserveAddr) - balanceReserveBefore, reserveAddr);

TimelockControllerEmergency.sol

scheduleBatch: targets.length should be stored in a variable

In the scheduleBatch function on line 221, we call targets.length to compare the targets length and also to loop through the targets and emit the CallScheduled event.

For a better code legibility and also to save some mload, targets length should be stored in a variable.

Recommendation:

uint256 _targetsLength = targets.length;

require(targetsLength == values.length, "TimelockController: length mismatch");
require(targetsLength == datas.length, "TimelockController: length mismatch");

bytes32 id = hashOperationBatch(targets, values, datas, predecessor, salt);
schedule(id, delay);
for (uint256 i = 0; i < targetsLength; ++i) {
  emit CallScheduled(id, i, targets[i], values[i], datas[i], predecessor, delay);
}

executeBatch: targets.length should be stored in a variable

In the executeBatch function on line 312, we call targets.length to compare the targets length and also to loop through the targets and call the _call function.

For a better code legibility and also to save some mload, targets length should be stored in a variable.

Recommendation:

uint256 _targetsLength = targets.length;

require(_targetsLength == values.length, "TimelockController: length mismatch");
require(_targetsLength == datas.length, "TimelockController: length mismatch");

bytes32 id = hashOperationBatch(targets, values, datas, predecessor, salt);
_beforeCall(id, predecessor);
for (uint256 i = 0; i < _targetsLength; ++i) {
    _call(id, i, targets[i], values[i], datas[i]);
}
_afterCall(id);