Nested Finance contest - MiloTruck's results

QA Report

Non-Critical Issues

Use of block.timestamp

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Recommended Mitigation Steps
Block timestamps should not be used for entropy or generating random numbers — i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

Instances where block.timestamp is used:

contracts/NestedFactory.sol:
 104:        /// The block.timestamp must be greater than NFT record lock timestamp
 107:        require(block.timestamp > nestedRecords.getLockTimestamp(_nftId), "NF: LOCKED_NFT");

contracts/governance/TimelockControllerEmergency.sol:
 135:        return timestamp > _DONE_TIMESTAMP && timestamp <= block.timestamp;
 245:        _timestamps[id] = block.timestamp + delay;

contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:
 169:        IBiswapRouter02(router).swapExactTokensForTokens(tokenAmountIn, 0, path, address(this), block.timestamp);
 243:        block.timestamp
 254:        block.timestamp

contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:
 169:        IUniswapV2Router02(router).swapExactTokensForTokens(tokenAmountIn, 0, path, address(this), block.timestamp);
 243:        block.timestamp
 254:        block.timestamp

event is missing indexed fields

Each event should use three indexed fields if there are three or more fields:

contracts/governance/TimelockControllerEmergency.sol:
  37:        event CallScheduled(
  38:            bytes32 indexed id,
  39:            uint256 indexed index,
  40:            address target,
  41:            uint256 value,
  42:            bytes data,
  43:            bytes32 predecessor,
  44:            uint256 delay
  45:        );

  50:        event CallExecuted(bytes32 indexed id, uint256 indexed index, address target, uint256 value, bytes data);

Gas Optimizations Report

For-loops: Index initialized with default value

Uninitialized uint variables are assigned with a default value of 0.

Thus, in for-loops, explicitly initializing an index with 0 costs unnecesary gas. For example, the following code:

for (uint256 i = 0; i < length; ++i) {

can be changed to:

for (uint256 i; i < length; ++i) {

Consider declaring the following lines without explicitly setting the index to 0:

contracts/NestedFactory.sol:
 124:        for (uint256 i = 0; i < operatorsCache.length; i++) {
 136:        for (uint256 i = 0; i < operatorsLength; i++) {
 196:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
 256:        for (uint256 i = 0; i < tokensLength; i++) {
 315:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
 333:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
 369:        for (uint256 i = 0; i < batchLength; i++) {
 412:        for (uint256 i = 0; i < batchLength; i++) {
 651:        for (uint256 i = 0; i < _batchedOrders.length; i++) {

contracts/OperatorResolver.sol:
  40:        for (uint256 i = 0; i < namesLength; i++) {
  60:        for (uint256 i = 0; i < names.length; i++) {
  75:        for (uint256 i = 0; i < destinations.length; i++) {

contracts/governance/TimelockControllerEmergency.sol:
  84:        for (uint256 i = 0; i < proposers.length; ++i) {
  89:        for (uint256 i = 0; i < executors.length; ++i) {
 234:        for (uint256 i = 0; i < targets.length; ++i) {
 324:        for (uint256 i = 0; i < targets.length; ++i) {

contracts/abstracts/MixinOperatorResolver.sol:
  37:        for (uint256 i = 0; i < requiredOperators.length; i++) {
  56:        for (uint256 i = 0; i < requiredOperators.length; i++) {

For-Loops: Index increments can be left unchecked

From Solidity v0.8 onwards, all arithmetic operations come with implicit overflow and underflow checks.

In for-loops, as it is impossible for the index to overflow, it can be left unchecked to save gas every iteration.

For example, the code below:

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

can be changed to:

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

Consider making the following change to these lines:

contracts/NestedFactory.sol:
 124:        for (uint256 i = 0; i < operatorsCache.length; i++) {
 136:        for (uint256 i = 0; i < operatorsLength; i++) {
 196:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
 256:        for (uint256 i = 0; i < tokensLength; i++) {
 315:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
 333:        for (uint256 i = 0; i < batchedOrdersLength; i++) {
 369:        for (uint256 i = 0; i < batchLength; i++) {
 412:        for (uint256 i = 0; i < batchLength; i++) {
 651:        for (uint256 i = 0; i < _batchedOrders.length; i++) {

contracts/OperatorResolver.sol:
  40:        for (uint256 i = 0; i < namesLength; i++) {
  60:        for (uint256 i = 0; i < names.length; i++) {
  75:        for (uint256 i = 0; i < destinations.length; i++) {

contracts/governance/TimelockControllerEmergency.sol:
  84:        for (uint256 i = 0; i < proposers.length; ++i) {
  89:        for (uint256 i = 0; i < executors.length; ++i) {
 234:        for (uint256 i = 0; i < targets.length; ++i) {
 324:        for (uint256 i = 0; i < targets.length; ++i) {

contracts/governance/scripts/OperatorScripts.sol:
  67:        for (uint256 i; i < operatorLength; i++) {
  80:        for (uint256 i; i < operatorLength; i++) {

contracts/libraries/CurveHelpers/CurveHelpers.sol:
  22:        for (uint256 i; i < 2; i++) {
  42:        for (uint256 i; i < 3; i++) {
  62:        for (uint256 i; i < 4; i++) {
  86:        for (uint256 i; i < poolCoinAmount; i++) {

contracts/abstracts/MixinOperatorResolver.sol:
  37:        for (uint256 i = 0; i < requiredOperators.length; i++) {
  56:        for (uint256 i = 0; i < requiredOperators.length; i++) {

contracts/operators/Yearn/YearnCurveVaultOperator.sol:
  42:        for (uint256 i; i < vaultsLength; i++) {

contracts/operators/Beefy/BeefyVaultOperator.sol:
  18:        for (uint256 i; i < vaultsLength; i++) {

contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:
  27:        for (uint256 i; i < vaultsLength; i++) {

contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:
  27:        for (uint256 i; i < vaultsLength; i++) {

Arithmetics: Use != 0 instead of > 0 for unsigned integers

uint will never go below 0. Thus, > 0 is gas inefficient in comparisons as checking if != 0 is sufficient and costs less gas.

Consider changing > 0 to != 0 in these lines:

contracts/governance/TimelockControllerEmergency.sol:
 120:        return getTimestamp(id) > 0;

Arithmetics: Use Shift Right/Left instead of Division/Multiplication if possible

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

For example, the following code:

uint256 b = a / 2;
uint256 c = a / 4;
uint256 d = a * 8;

can be changed to:

uint256 b = a >> 1;
uint256 c = a >> 2;
uint256 d = a << 3;

Consider making this change to the following lines:

contracts/operators/Beefy/lp/BeefyZapBiswapLPVaultOperator.sol:
 275:        uint256 halfInvestment = investmentA / 2;

contracts/operators/Beefy/lp/BeefyZapUniswapLPVaultOperator.sol:
 273:        uint256 halfInvestment = investmentA / 2;

Visibility: Consider declaring constants as non-public to save gas

If a constant is not used outside of its contract, declaring it as private or internal instead of public can save gas.

Consider changing the visibility of the following from public to internal or private:

contracts/governance/TimelockControllerEmergency.sol:
  25:        bytes32 public constant TIMELOCK_ADMIN_ROLE = keccak256("TIMELOCK_ADMIN_ROLE");
  26:        bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE");
  27:        bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");
  28:        bytes32 public constant EMERGENCY_ROLE = keccak256("EMERGENCY_ROLE");

Visibility: public functions can be set to external

Calls to external functions are cheaper than public functions. Thus, if a function is not used internally in any contract, it should be set to external to save gas and improve code readability.

Consider changing following functions from public to external:

contracts/governance/OwnerProxy.sol:
  16:        function execute(address _target, bytes memory _data) public payable onlyOwner returns (bytes memory response) {

contracts/governance/TimelockControllerEmergency.sol:
 295:        function executeEmergency(
 296:            address target,
 297:            uint256 value,
 298:            bytes calldata data
 299:        ) public payable onlyRole(EMERGENCY_ROLE) {

Errors: Reduce the length of error messages (long revert strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

In these instances, consider shortening the revert strings to fit within 32 bytes, or using custom errors:

contracts/governance/TimelockControllerEmergency.sol:
 229:        require(targets.length == values.length, "TimelockController: length mismatch");
 230:        require(targets.length == datas.length, "TimelockController: length mismatch");
 243:        require(!isOperation(id), "TimelockController: operation already scheduled");
 244:        require(delay >= getMinDelay(), "TimelockController: insufficient delay");
 256:        require(isOperationPending(id), "TimelockController: operation cannot be cancelled");
 319:        require(targets.length == values.length, "TimelockController: length mismatch");
 320:        require(targets.length == datas.length, "TimelockController: length mismatch");
 334:        require(isOperationReady(id), "TimelockController: operation is not ready");
 335:        require(predecessor == bytes32(0) || isOperationDone(predecessor), "TimelockController: missing dependency");
 342:        require(isOperationReady(id), "TimelockController: operation is not ready");
 359:        require(success, "TimelockController: underlying transaction reverted");
 375:        require(msg.sender == address(this), "TimelockController: caller must be timelock");

Variables declared as constant are expressions, not constants

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232:

Consequences: each usage of a “constant” costs ~100 gas more on each access (it is still a little better than storing the result in storage, but not much). since these are not real constants, they can’t be referenced from a real constant environment (e.g. from assembly, or from another library)

contracts/abstracts/OwnableProxyDelegation.sol:
  15:        bytes32 internal constant _ADMIN_SLOT = bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1);

Change these expressions from constant to immutable and implement the calculation in the constructor. Alternatively, hardcode these values in the constants and add a comment to say how the value was calculated.