InsureDAO contest - 0xngndev's results

Handle

0xngndev

Vulnerability details

Impact

In Vault.sol when Controller is set to address(0) and available() is less than the amount the user wants to withdraw, the function withdrawValue will revert. This happens because when available() < _amount the internal function _unutilize is called, and that internal function has a check that reverts if the controller is set to the zero address. This very same thing happens with _withdrawAttribution

I’m not aware whether this is a conscious decision, or if I’m missing something, but I thought I’d bring it up just in case it wasn’t. Although I understand this is an edge case and shouldn’t happen often, and the user could simply reduce the amount it wants to withdraw to avoid entering into the block that causes _unutilize to be called, I believe it may be quite troublesome for a user to figure out the true error in a real situation as the error he would face is ERROR_CONTROLLER_NOT_SET, instead of something like AMOUNT_EXCEEDS_RESERVES, which would prompt the user to withdraw a lower amount.

Handle

0xngndev

Vulnerability details

Impact

Unclear Natspec may confuse the user.

In the fund function:

• https://github.com/code-423n4/2022-01-insure/blob/main/contracts/CDSTemplate.sol#L160

The Natspec is a copy-paste of the deposit function:

• https://github.com/code-423n4/2022-01-insure/blob/main/contracts/CDSTemplate.sol#L130

The problem here is the receives ITokens part of the Natspec. The deposit function indeed mints tokens to the msg.sender but the fund function doesn’t. I would clarify that the fund function adds attributions to the surplusPool.

Another minor and unclear bit of Natspec happens here: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L177

It describes _amount as sender of value instead of something like amount of value to send.

Recommended Mitigation Steps

Explain the Natspec of the fund function in more detail. Fix the transferValue amount natspec. Also it would be good to add some Natspec to the defund function too.

Handle

0xngndev

Vulnerability details

Impact

Removing require for assert can save gas on failing conditions. The assert is present here:

https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L168

Proof of Concept

To test this I created two different contracts, one that uses asserts, the other that uses require and tested the difference in gas when these functions fail.

Contracts

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

contract Assert {
  function assertFunc(uint256 _amount) public pure returns (uint256) {
    assert(_amount == 5);
    return 5;
  }
}

pragma solidity 0.8.10;

contract AssertCounter {
  function requireFunc(uint256 _amount) public pure returns (uint256) {
    require(_amount == 5);
    return 5;
  }
}

Tests:

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

import "ds-test/test.sol";
import "../Assert.sol";
import "../AssertCounter.sol";

contract AssertTest is DSTest {
  Assert assertContract;
  AssertCounter assertCounterContract;

  function setUp() public {
    assertContract = new Assert();
    assertCounterContract = new AssertCounter();
  }

  function testFailAssert() public logs_gas {
    assertContract.assertFunc(3);
  }

  function testFailRequire() public logs_gas {
    assertCounterContract.requireFunc(3);
  }

}

Results:

• TestFailRequire: 720 gas
• TestFailAssert: 802 gas

Tools Used

Dapptools/Foundry

Recommended Mitigation Steps

Replace the assert with a revert or require

Handle

0xngndev

Vulnerability details

Impact

In PoolTemplate.sol there are multiple instances where variables are declared before the error checks of the functions. In cases where a function reverts due to these error checks, that extra computation of calculating the variable being declared can be avoided by simply moving the declaration after the error checks.

Here are all the functions I found where this can be applied:

• withdraw function: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L293
• withdrawCredit function: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L416
• insure function: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L465
• reedem function: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L548

Recommended Mitigation Steps

• Change withdraw function to:

function withdraw(uint256 _amount) external returns (uint256 _retVal) {
  require(
      marketStatus == MarketStatus.Trading,
      "ERROR: WITHDRAWAL_PENDING"
  );
  require(
      withdrawalReq[msg.sender].timestamp +
          parameters.getLockup(msg.sender) <
          block.timestamp,
      "ERROR: WITHDRAWAL_QUEUE"
  );
  require(
      withdrawalReq[msg.sender].timestamp +
          parameters.getLockup(msg.sender) +
          parameters.getWithdrawable(msg.sender) >
          block.timestamp,
      "ERROR: WITHDRAWAL_NO_ACTIVE_REQUEST"
  );
  require(
      withdrawalReq[msg.sender].amount >= _amount,
      "ERROR: WITHDRAWAL_EXCEEDED_REQUEST"
  );
  require(_amount > 0, "ERROR: WITHDRAWAL_ZERO");
  require(
      _retVal <= availableBalance(),
      "ERROR: WITHDRAW_INSUFFICIENT_LIQUIDITY"
  );

  uint256 _supply = totalSupply();
  require(_supply != 0, "ERROR: NO_AVAILABLE_LIQUIDITY");

  uint256 _liquidity = originalLiquidity();
  _retVal = (_amount * _liquidity) / _supply;

  //reduce requested amount
  withdrawalReq[msg.sender].amount -= _amount;

  //Burn iToken
  _burn(msg.sender, _amount);

  //Withdraw liquidity
  vault.withdrawValue(_retVal, msg.sender);

  emit Withdraw(msg.sender, _amount, _retVal);
}

• Change withdrawCredit function to:

function withdrawCredit(uint256 _credit)
        external
        override
        returns (uint256 _pending)
    {
      IndexInfo storage _index = indicies[msg.sender];
      require(
          IRegistry(registry).isListed(msg.sender) &&
              _index.credit >= _credit &&
              _credit <= availableBalance(),
          "ERROR: WITHDRAW_CREDIT_BAD_CONDITIONS"
      );

      uint256 _rewardPerCredit = rewardPerCredit;

      //calculate acrrued premium
      _pending = _sub(
          (_index.credit * _rewardPerCredit) / MAGIC_SCALE_1E6,
          _index.rewardDebt
      );

      //Withdraw liquidity
      if (_credit > 0) {
          totalCredit -= _credit;
          _index.credit -= _credit;
          emit CreditDecrease(msg.sender, _credit);
      }

      //withdraw acrrued premium
      if (_pending > 0) {
          vault.transferAttribution(_pending, msg.sender);
          attributionDebt -= _pending;
          _index.rewardDebt =
              (_index.credit * _rewardPerCredit) /
              MAGIC_SCALE_1E6;
      }
}

• Change insure function to:

function insure(
        uint256 _amount,
        uint256 _maxCost,
        uint256 _span,
        bytes32 _target
    ) external returns (uint256) {
      //Distribute premium and fee
      uint256 _premium = getPremium(_amount, _span);

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

      uint256 _endTime = _span + block.timestamp;
      uint256 _fee = parameters.getFeeRate(msg.sender);

      //current liquidity
      uint256 _liquidity = totalLiquidity();
      uint256 _totalCredit = totalCredit;

      //accrue premium/fee
      uint256[2] memory _newAttribution = vault.addValueBatch(
          _premium,
          msg.sender,
          [address(this), parameters.getOwner()],
          [MAGIC_SCALE_1E6 - _fee, _fee]
      );

      //Lock covered amount
      uint256 _id = allInsuranceCount;
      lockedAmount += _amount;
      Insurance memory _insurance = Insurance(
          _id,
          block.timestamp,
          _endTime,
          _amount,
          _target,
          msg.sender,
          true
      );
      insurances[_id] = _insurance;
      allInsuranceCount += 1;

      //Calculate liquidity for index
      if (_totalCredit > 0) {
          uint256 _attributionForIndex = (_newAttribution[0] * _totalCredit) /
              _liquidity;
          attributionDebt += _attributionForIndex;
          rewardPerCredit += ((_attributionForIndex * MAGIC_SCALE_1E6) /
              _totalCredit);
      }

      emit Insured(
          _id,
          _amount,
          _target,
          block.timestamp,
          _endTime,
          msg.sender,
          _premium
      );

        return _id;
  }

• Change redeem function to:

function redeem(uint256 _id, bytes32[] calldata _merkleProof) external {
      require(
          marketStatus == MarketStatus.Payingout,
          "ERROR: NO_APPLICABLE_INCIDENT"
      );
      Insurance storage _insurance = insurances[_id];
      require(_insurance.status == true, "ERROR: INSURANCE_NOT_ACTIVE");
      require(_insurance.insured == msg.sender, "ERROR: NOT_YOUR_INSURANCE");
      uint256 _incidentTimestamp = incident.incidentTimestamp;
      require(
          marketStatus == MarketStatus.Payingout &&
              _insurance.startTime <= _incidentTimestamp &&
              _insurance.endTime >= _incidentTimestamp,
          "ERROR: INSURANCE_NOT_APPLICABLE"
      );
      bytes32 _targets = incident.merkleRoot;
      require(
          MerkleProof.verify(
              _merkleProof,
              _targets,
              keccak256(
                  abi.encodePacked(_insurance.target, _insurance.insured)
              )
          ) ||
              MerkleProof.verify(
                  _merkleProof,
                  _targets,
                  keccak256(abi.encodePacked(_insurance.target, address(0)))
              ),
          "ERROR: INSURANCE_EXEMPTED"
      );
      uint256 _payoutNumerator = incident.payoutNumerator;
      uint256 _payoutDenominator = incident.payoutDenominator;

      _insurance.status = false;
      lockedAmount -= _insurance.amount;

      uint256 _payoutAmount = (_insurance.amount * _payoutNumerator) /
          _payoutDenominator;

      vault.borrowValue(_payoutAmount, msg.sender);

      emit Redeemed(
          _id,
          msg.sender,
          _insurance.target,
          _insurance.amount,
          _payoutAmount
      );
  }

Handle

0xngndev

Vulnerability details

Impact

In PoolTemplate.sol you can group repetitive logic into a modifier to save on deployment costs. There’s a tradeoff to doing this though, so this is mostly a suggestion. The tradeoff is that declaring the logic inline is cheaper than having it in a modifier when you call the function that uses that modifier. So, you would be decluttering your code a bit and saving on deployment costs because your code size will be reduced, but calling the functions that implement the modifier will be slightly more expensive.

The logic that can be moved into a modifier is the following:

require(
    marketStatus == MarketStatus.Trading && paused == false,
    "ERROR: DEPOSIT_DISABLED"
);

This logic is used three times across PoolTemplate.sol:

• https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L232
• https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L255
• insure function: here you had the paused as a separate requirements instead of in the same, you could group it together.
  • https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L465

Recommended Mitigation Steps

If you are okay with the tradeoff, move the logic stated above into its own modifier and apply it to the functions that make the marketStatus and paused checks.

Handle

0xngndev

Vulnerability details

Impact

Replacing condition == false for !condition and condition == true for conditionsaves gas. Example of where this can be applied:

https://github.com/code-423n4/2022-01-insure/blob/main/contracts/CDSTemplate.sol#L131

There are many instances where == false or == true are used, a control + shift + f through the repo should show them fast.

Proof of Concept

To test this I created two different contracts, one that uses == false, the other that uses !condition and tested the difference in gas when the functions are called.

Contracts

pragma solidity 0.8.10;

contract Paused {
  bool public paused;

  function a() public view returns (uint256) {
    require(paused == false, "PAUSED");
    return 5;
  }
}

pragma solidity 0.8.10;

contract PausedCounter {
  bool public paused;

  function a() public view returns (uint256) {
    require(!paused, "PAUSED");
    return 5;
  }
}

pragma solidity 0.8.10;

contract PausedTrue {
  bool public paused = true;

  function a() public view returns (uint256) {
    require(paused == true, "PAUSED");
    return 5;
  }
}

pragma solidity 0.8.10;

contract PausedTrueCounter {
  bool public paused = true;

  function a() public view returns (uint256) {
    require(paused, "PAUSED");
    return 5;
  }
}

Tests:

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

import "ds-test/test.sol";
import "../Paused.sol";
import "../PausedCounter.sol";
import "../PausedTrueCounter.sol";
import "../PausedTrue.sol";

contract PausedTest is DSTest {
  Paused pausedContract;
  PausedCounter pausedCounterContract;
  PausedTrue pausedTrueContract;
  PausedTrueCounter pausedTrueCounterContract;

  function setUp() public {
    pausedContract = new Paused();
    pausedCounterContract = new PausedCounter();
    pausedTrueContract = new PausedTrue();
    pausedTrueCounterContract = new PausedTrueCounter();
  }

  function testPaused() public logs_gas {
    pausedContract.a();
  }

  function testPausedCounter() public logs_gas {
    pausedCounterContract.a();
  }

  function testPausedTrue() public logs_gas {
    pausedTrueContract.a();
  }

  function testPausedTrueCounter() public logs_gas {
    pausedTrueCounterContract.a();
  }
}

Results:

• testPaused (uses == false): 2841 gas
• testPausedCounter (uses !condition): 2791 gas
• testPausedTrue(uses == true): 800 gas
• testPausedTrueCounter (uses condition): 788 gas

Tools Used

Dapptools/Foundry

Recommended Mitigation Steps

Replace == false and == true with !condition and condition

Handle

0xngndev

Vulnerability details

Impact

Running a quick search through the code unveiled that there are many instances where you are using >0 checks for uints. This is good, but you can save a small amount of gas by turning them into !=0.

Recommended Mitigation Steps

Change >0 to !=0 for the uint256 checks.

Handle

0xngndev

Vulnerability details

Impact

Error Messages that have a length of 32 or more one require one additional slot to be stored, causing extra gas costs when deploying the contract and when the function is executed and it reverts.

Proof of Concept

I put together a quick proof to show the different impact of the errors we can have in Solidity:

• Long require errors => more than 32 bytes
• Short require errors => less than 32 bytes
• Custom errors

Here are the contract size findings:

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

contract Errors {
  bool public thisIsFalse;
  error WithdrawalExceeded();

  /*
    Contract Size with just this function: 333 bytes;
  */
  function moreThan32Bytes() public {
    require(thisIsFalse, "ERROR: WITHDRAWAL_EXCEEDED_REQUEST");
  }

  /*
    Contract Size with just this function: 295 bytes;
//   */
  function lessThan32Bytes() public {
    require(thisIsFalse, "WITHDRAWAL_EXCEEDED_REQUEST");
  }

  /*
    Contract Size with just this function: 242 bytes;
  */
  function customError() public {
    if (!thisIsFalse) revert WithdrawalExceeded();
  }
}

I then run tests to see the gas costs of having the functions revert, and although these are not very accurate due to the fact that it’s hard to isolate the gas costs of a reverting function due to the order of execution (I can’t have an event that logs the gas before the function revert and another one after because the one after the revert will never be reached), it still shows some differences.

//SPDX-License-Identifier: unlicensed
pragma solidity 0.8.10;

import "ds-test/test.sol";
import "../Errors.sol";

contract ErrorsTest is DSTest {
  Errors errors;

  function setUp() public {
    errors = new Errors();
  }

  function testFailLessThan32Bytes() public logs_gas {
    errors.lessThan32Bytes();
  }

  function testFailMoreThan32Bytes() public logs_gas {
    errors.moreThan32Bytes();
  }

  function testFailCustomError() public logs_gas {
    errors.customError();
  }
}

Running 3 tests for "ErrorsTest.json":ErrorsTest
[PASS] testFailCustomError() (gas: 3161)
[PASS] testFailLessThan32Bytes() (gas: 3314)
[PASS] testFailMoreThan32Bytes() (gas: 3401)

Tools Used

DappTools/Foundry

Recommended Mitigation Steps

Personally, I would switch to custom errors and reverts to maximize the savings, but if you dislike revert syntax, then I would suggest to check which of your require errors have a length longer than 32, and shorten them so that their length is less than 32.

Here are some examples of the errors you could shorten in your CDSTemplate.sol contract:

• ERROR: INITIALIZATION_BAD_CONDITIONS
• ERROR: WITHDRAWAL_NO_ACTIVE_REQUEST
• ERROR: WITHDRAWAL_EXCEEDED_REQUEST

Removing the “ERROR” keyword should be enough for most of these. Bear in mind you can always have concise error messages and a section in your documentation that explains them further or have your natspec expand on them if you find them too cryptic.

An example of how to apply a custom error in the first error would be to just have the error say BadConditions(). The user knows it’s an error because the function call failed, and the user knows it has happened in the initialize function because he called it, so BadConditions() should be a clear message despite being concise

Handle

0xngndev

Vulnerability details

Impact

In CDSTemplate.sol#withdraw #L238 because request is declared as a memory variable of Withdrawal #L209, when it updates request.amount -= _amount the update is not being stored after the function is called, which completely invalidates the following check at #L227:

require(
    request.amount >= _amount,
    "ERROR: WITHDRAWAL_EXCEEDED_REQUEST"
);

I don’t think funds are at risk due to the burn function being called afterwards, which would throw an error if the user tries to withdraw more collateral that he has, so low seemed like the correct risk assessment.

Recommended Mitigation Steps

Either change #L238 for:

withdrawalReq[msg.sender].amount -= _amount

or change the declaration at #L209 for:

Withdrawal storage request = withdrawalReq[msg.sender];