code-423n4 / 2022-01-insure-findings

2 stars 0 forks source link

Order of statements #332

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pauliax

Vulnerability details

Impact

Statements should be ordered in a way that it costs less gas, that is, less operations are performed when the validating conditions are wrong. e.g. this can be reordered:

  //Distribute premium and fee
  uint256 _endTime = _span + block.timestamp;
  uint256 _premium = getPremium(_amount, _span);
  uint256 _fee = parameters.getFeeRate(msg.sender);

  require(
      _amount <= availableBalance(),
      "ERROR: INSURE_EXCEEDED_AVAILABLE_BALANCE"
  );
  require(_premium <= _maxCost, "ERROR: INSURE_EXCEEDED_MAX_COST");
  require(_span <= 365 days, "ERROR: INSURE_EXCEEDED_MAX_SPAN");
  require(
      parameters.getMinDate(msg.sender) <= _span,
      "ERROR: INSURE_SPAN_BELOW_MIN"
  );

  require(
      marketStatus == MarketStatus.Trading,
      "ERROR: INSURE_MARKET_PENDING"
  );
  require(paused == false, "ERROR: INSURE_MARKET_PAUSED");

to something like this:

  require(paused == false, "ERROR: INSURE_MARKET_PAUSED");
  require(
      marketStatus == MarketStatus.Trading,
      "ERROR: INSURE_MARKET_PENDING"
  );

  require(
      _amount <= availableBalance(),
      "ERROR: INSURE_EXCEEDED_AVAILABLE_BALANCE"
  );

  require(_span <= 365 days, "ERROR: INSURE_EXCEEDED_MAX_SPAN");
  require(
      parameters.getMinDate(msg.sender) <= _span,
      "ERROR: INSURE_SPAN_BELOW_MIN"
  );

  //Distribute premium and fee
  uint256 _premium = getPremium(_amount, _span);
  require(_premium <= _maxCost, "ERROR: INSURE_EXCEEDED_MAX_COST");

  uint256 _endTime = _span + block.timestamp;
  uint256 _fee = parameters.getFeeRate(msg.sender);
takadr commented 2 years ago

@oishun1112 When I change the code, 1 test case failed.

AssertionError: Expected transaction to be reverted with ERROR: INSURE_SPAN_BELOW_MIN, but other exception was thrown: Error: VM Exception while processing transaction: reverted with reason string 'ERROR: INSURE_MARKET_PENDING'

https://github.com/code-423n4/2022-01-insure/blob/main/test/unitary/PoolTemplate/pool.test.js#L1960

I'm not sure what behavior is correct.

oishun1112 commented 2 years ago

this is a revert test when insurance market is in payout status.

please delete these lines https://github.com/code-423n4/2022-01-insure/blob/main/test/unitary/PoolTemplate/pool.test.js#L1956-L1960