search

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

4 stars 0 forks source link

QA Report #334

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago
Overview Risk Rating Number of issues
Low Risk 7
Non-Critical Risk 4

Table of Contents

Low Risk Issues

1. Missing address(0) checks

Consider adding an address(0) check for immutable variables:

55:  address public immutable WETH;
...
104:   constructor(address _WETH, address _matchExecutor) {
...
+ 115:     require(_WETH != address(0));
115:     WETH = _WETH;
...
117:   }

2. Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

File: InfinityExchange.sol
1220:   function rescueTokens(
1221:     address destination,
1222:     address currency,
1223:     uint256 amount
1224:   ) external onlyOwner {
+ 1225:     require(destination != address(0));
1225:     IERC20(currency).safeTransfer(destination, amount);
1226:   }

3. Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock to:

core/InfinityExchange.sol:1266:  function setProtocolFee(uint16 _protocolFeeBps) external onlyOwner {

4. abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()

Similar issue in the past: here

Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.

core/InfinityExchange.sol:1177:          keccak256(abi.encodePacked(order.constraints)),
core/InfinityExchange.sol:1179:          keccak256(abi.encodePacked(order.execParams)),
core/InfinityExchange.sol:1197:    bytes32 nftsHash = keccak256(abi.encodePacked(hashes));
core/InfinityExchange.sol:1213:    bytes32 tokensHash = keccak256(abi.encodePacked(hashes));
libs/SignatureChecker.sol:61:    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, orderHash));
token/TimelockConfig.sol:77:    return keccak256(abi.encodePacked(name));

5. Events not indexed

core/InfinityExchange.sol:80:  event CancelAllOrders(address user, uint256 newMinNonce);
core/InfinityExchange.sol:81:  event CancelMultipleOrders(address user, uint256[] orderNonces);
core/InfinityExchange.sol:85:  event MatchOrderFulfilled(
core/InfinityExchange.sol:95:  event TakeOrderFulfilled(
token/TimelockConfig.sol:34:  event ChangeRequested(bytes32 configId, uint256 value);
token/TimelockConfig.sol:35:  event ChangeConfirmed(bytes32 configId, uint256 value);
token/TimelockConfig.sol:36:  event ChangeCanceled(bytes32 configId, uint256 value);

6. Use a constant instead of duplicating the same string

core/InfinityExchange.sol:138:    require(msg.sender == MATCH_EXECUTOR, 'OME');
core/InfinityExchange.sol:183:    require(msg.sender == MATCH_EXECUTOR, 'OME');
core/InfinityExchange.sol:263:    require(msg.sender == MATCH_EXECUTOR, 'OME');
core/InfinityExchange.sol:155:      require(canExec, 'cannot execute');
core/InfinityExchange.sol:187:      'cannot execute'
core/InfinityExchange.sol:279:      require(executionValid, 'cannot execute');
core/InfinityExchange.sol:150:      require(_complications.contains(makerOrders1[i].execParams[0]), 'invalid complication');
core/InfinityExchange.sol:184:    require(_complications.contains(makerOrder.execParams[0]), 'invalid complication');
core/InfinityExchange.sol:190:    require(isOrderValid(makerOrder, makerOrderHash), 'invalid maker order');
core/InfinityExchange.sol:310:      require(isOrderValid(makerOrders[i], makerOrderHash), 'invalid maker order');
core/InfinityExchange.sol:139:    require(numMakerOrders == makerOrders2.length, 'mismatched lengths');
core/InfinityExchange.sol:264:    require(numSells == buys.length && numSells == constructs.length, 'mismatched lengths');
core/InfinityExchange.sol:342:    require(ordersLength == takerNfts.length, 'mismatched lengths');
core/InfinityExchange.sol:380:    require(minNonce > userMinOrderNonce[msg.sender], 'nonce too low');
core/InfinityExchange.sol:394:      require(orderNonces[i] >= userMinOrderNonce[msg.sender], 'nonce too low');
core/InfinityExchange.sol:587:    require(verifyMatchOneToOneOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
core/InfinityExchange.sol:621:    require(verifyMatchOneToManyOrders(buyOrderHash, false, sell, buy), 'order not verified');
core/InfinityExchange.sol:649:    require(verifyMatchOneToManyOrders(sellOrderHash, true, sell, buy), 'order not verified');
core/InfinityExchange.sol:684:    require(verifyMatchOrders(sellOrderHash, buyOrderHash, sell, buy), 'order not verified');
core/InfinityExchange.sol:949:    require(makerOrderValid && executionValid, 'order not verified');
staking/InfinityStaker.sol:68:    require(amount != 0, 'stake amount cant be 0');
staking/InfinityStaker.sol:117:    require(amount != 0, 'stake amount cant be 0');

7. Use a 2-step ownership transfer pattern

Contracts inheriting from OpenZeppelin's libraries have the default transferOwnership() function (a one-step process). It's possible that the onlyOwner role mistakenly transfers ownership to a wrong address, resulting in a loss of the onlyOwner role. Consider overriding the default transferOwnership() function to first nominate an address as the pendingOwner and implementing an acceptOwnership() function which is called by the pendingOwner to confirm the transfer.

core/InfinityExchange.sol:5:import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
core/InfinityExchange.sol:50:contract InfinityExchange is ReentrancyGuard, Ownable {

Non-Critical Issues

1. Typos

core/InfinityExchange.sol:58:  /// @dev This is the adress that is used to send auto sniped orders for execution on chain
core/InfinityExchange.sol:77:  /// @dev Storate variable that keeps track of valid currencies (tokens)
libs/OrderTypes.sol:39:    ///@dev additional parameters like traits for trait orders, private sale buyer for OTC ordes etc
core/InfinityExchange.sol:1260:  function updateWethTranferGas(uint32 _wethTransferGasUnits) external onlyOwner {
libs/SignatureChecker.sol:14:   * @param hashed the hash containing the signed mesage
libs/SignatureChecker.sol:48:   * @param domainSeparator paramer to prevent signature being executed in other chains and environments

2. Use scientific notation for readability reasons

100000 should be changed to 1e5 1000000 should be changed to 1e6 20000 should be changed to 2e5 50000 should be changed to 5e5

core/InfinityExchange.sol:61:  uint32 public WETH_TRANSFER_GAS_UNITS = 50000;
core/InfinityExchange.sol:201:        // 20000 for the SSTORE op that updates maker nonce status from zero to a non zero status
core/InfinityExchange.sol:202:        uint256 startGasPerOrder = gasleft() + ((startGas + 20000 - gasleft()) / ordersLength);
core/InfinityExchange.sol:381:    require(minNonce < userMinOrderNonce[msg.sender] + 1000000, 'too many');
core/InfinityExchange.sol:725:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:775:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:819:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:873:    uint256 protocolFee = (protocolFeeBps * execPrice) / 10000;
core/InfinityExchange.sol:1135:    uint256 protocolFee = (PROTOCOL_FEE_BPS * amount) / 10000;
staking/InfinityStaker.sol:35:  uint16 public GOLD_STAKE_THRESHOLD = 10000;
staking/InfinityStaker.sol:36:  uint16 public PLATINUM_STAKE_THRESHOLD = 20000;

3. Use string.concat() or bytes.concat()

Solidity version 0.8.4 introduces bytes.concat() (vs abi.encodePacked(<bytes>,<bytes>)) Solidity version 0.8.12 introduces string.concat() (vs abi.encodePacked(<str>,<str>))

Here, the version is 0.8.14:

core/InfinityExchange.sol:1177:          keccak256(abi.encodePacked(order.constraints)),
core/InfinityExchange.sol:1179:          keccak256(abi.encodePacked(order.execParams)),
core/InfinityExchange.sol:1197:    bytes32 nftsHash = keccak256(abi.encodePacked(hashes));
core/InfinityExchange.sol:1213:    bytes32 tokensHash = keccak256(abi.encodePacked(hashes));
libs/SignatureChecker.sol:61:    bytes32 digest = keccak256(abi.encodePacked('\x19\x01', domainSeparator, orderHash));
libs/SignatureChecker.sol:65:      return IERC1271(signer).isValidSignature(digest, abi.encodePacked(r, s, v)) == 0x1626ba7e;
token/TimelockConfig.sol:77:    return keccak256(abi.encodePacked(name));

4. Adding a return statement when the function defines a named return variable, is redundant

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity.

Affected code:

contracts/token/InfinityToken.sol:
  113:   function getAdmin() public view returns (address admin) {
  114:     return address(uint160(TimelockConfig.getConfig(TimelockConfig.ADMIN).value));

  117:   function getTimelock() public view returns (uint256 timelock) {
  118:     return TimelockConfig.getConfig(TimelockConfig.TIMELOCK).value;

  121:   function getInflation() public view returns (uint256 inflation) {
  122:     return TimelockConfig.getConfig(EPOCH_INFLATION).value;

  125:   function getCliff() public view returns (uint256 cliff) {
  126:     return TimelockConfig.getConfig(EPOCH_CLIFF).value;

  129:   function getMaxEpochs() public view returns (uint256 totalEpochs) {
  130:     return TimelockConfig.getConfig(MAX_EPOCHS).value;

  133:   function getEpochDuration() public view returns (uint256 epochDuration) {
  134:     return TimelockConfig.getConfig(EPOCH_DURATION).value;

contracts/token/TimelockConfig.sol:
   76:   function calculateConfigId(string memory name) external pure returns (bytes32 configId) {
   77:     return keccak256(abi.encodePacked(name));

   80:   function isConfig(bytes32 configId) external view returns (bool status) {
   81:     return _configSet.contains(configId);

   84:   function getConfigCount() external view returns (uint256 count) {
   85:     return _configSet.length();

   88:   function getConfigByIndex(uint256 index) external view returns (Config memory config) {
   90:     return Config(configId, _config[configId]);

   93:   function getConfig(bytes32 configId) public view returns (Config memory config) {
   95:     return Config(configId, _config[configId]);

   98:   function isPending(bytes32 configId) public view returns (bool status) {
   99:     return _pendingSet.contains(configId);

  102:   function getPendingCount() external view returns (uint256 count) {
  103:     return _pendingSet.length();

  106:   function getPendingByIndex(uint256 index) external view returns (PendingChange memory pendingRequest) {
  108:     return PendingChange(configId, _pending[configId].value, _pending[configId].timestamp);

  111:   function getPending(bytes32 configId) external view returns (PendingChange memory pendingRequest) {
  113:     return PendingChange(configId, _pending[configId].value, _pending[configId].timestamp);
nneverlander commented 2 years ago

Thanks

nneverlander commented 2 years ago

Thanks