code423n4 commented 2 years ago

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);

obatirou commented 2 years ago

Use of block.timestamp (disputed)

Not used for generating random numbers. Time-sensitive logic. And already surfaced in previous audit.

obatirou commented 2 years ago

event is missing indexed fields (duplicate)

Duplicate from #11

code-423n4 / 2022-06-nested-findings