InsureDAO contest - robee's results

Handle

robee

Vulnerability details

Not verified address arguments of external/public functions is a low risk issue. It's less severe for onlyOwner methods but for any other method it's crucial since the default address is 0.

    Argument _owner of CDSTemplate.sol.valueOfUnderlying is not verified to be != 0
    Argument from of CDSTemplate.sol._beforeTokenTransfer is not verified to be != 0
    Argument to of CDSTemplate.sol._beforeTokenTransfer is not verified to be != 0
    Argument _registry of Factory.sol.constructor is not verified to be != 0
    Argument _ownership of Factory.sol.constructor is not verified to be != 0
    Argument _target of Factory.sol.approveReference is not verified to be != 0
    Argument target of Factory.sol._createClone is not verified to be != 0
    Argument _owner of IndexTemplate.sol.valueOfUnderlying is not verified to be != 0

Handle

robee

Vulnerability details

The following requires are with empty messages. This is very important to add a message for any require. Such that the user has enough information to know the reason of failure:

    Solidity file: CDSTemplate.sol, In line 253 with Empty Require message.
    Solidity file: Factory.sol, In line 100 with Empty Require message.
    Solidity file: IndexTemplate.sol, In line 477 with Empty Require message.
    Solidity file: PoolTemplate.sol, In line 929 with Empty Require message.
    Solidity file: Vault.sol, In line 66 with Empty Require message.
    Solidity file: Vault.sol, In line 67 with Empty Require message.
    Solidity file: Vault.sol, In line 68 with Empty Require message.

Handle

robee

Vulnerability details

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    CDSTemplate.sol, totalLiquidity
    Factory.sol, _createClone
    IndexTemplate.sol, withdrawable
    IndexTemplate.sol, leverage
    IndexTemplate.sol, totalLiquidity
    PoolTemplate.sol, availableBalance
    PoolTemplate.sol, utilizationRate
    PoolTemplate.sol, totalLiquidity

Handle

robee

Vulnerability details

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    Ownership.sol

Handle

robee

Vulnerability details

From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix. With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam. You have reachable asserts in the following locations (which should be replaced by require / are mistakenly left from development phase):

    InsureDAOERC20.sol : reachable assert in line 31
    Vault.sol : reachable assert in line 167

Handle

robee

Vulnerability details

The project is compiled with different versions of solidity, which is not recommended due ti undefined behaviors as a result of it.

Handle

robee

Vulnerability details

In the following files there are contract imports that aren't used. Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore). The following is a full list of all unused imports, we went through the whole code to find it :) <solidity file> <line number> <actual import line>:

    Factory.sol, line 13, import "hardhat/console.sol";
    IndexTemplate.sol, line 6, import "hardhat/console.sol";
    Parameters.sol, line 12, import "hardhat/console.sol";
    BondingPremium.sol, line 9, import "@openzeppelin/contracts/utils/math/SafeMath.sol";

Handle

robee

Vulnerability details

The following functions could skip other steps if the amount is 0. (A similar issue: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/82)

    InsureDAOERC20.sol, name
    InsureDAOERC20.sol, symbol
    InsureDAOERC20.sol, decimals
    InsureDAOERC20.sol, totalSupply
    InsureDAOERC20.sol, balanceOf
    InsureDAOERC20.sol, transfer
    InsureDAOERC20.sol, allowance
    InsureDAOERC20.sol, approve
    InsureDAOERC20.sol, transferFrom
    InsureDAOERC20.sol, increaseAllowance
    InsureDAOERC20.sol, decreaseAllowance
    InsureDAOERC20.sol, _transfer
    InsureDAOERC20.sol, _mint
    InsureDAOERC20.sol, _burn
    InsureDAOERC20.sol, _approve
    InsureDAOERC20.sol, _beforeTokenTransfer
    InsureDAOERC20.sol, _afterTokenTransfer

Handle

robee

Vulnerability details

 Boolean variables can be checked within conditionals directly without the use of equality operators to true/false.

    CDSTemplate.sol, 99: initialized == false &&
    CDSTemplate.sol, 131: require(paused == false, "ERROR: PAUSED");
    CDSTemplate.sol, 161: require(paused == false, "ERROR: PAUSED");
    CDSTemplate.sol, 176: require(paused == false, "ERROR: PAUSED");
    CDSTemplate.sol, 205: require(paused == false, "ERROR: PAUSED");
    Factory.sol, 122: templates[address(_template)].approval == true,
    Factory.sol, 142: templates[address(_template)].approval == true,
    Factory.sol, 166: templates[address(_template)].approval == true,
    Factory.sol, 169: if (templates[address(_template)].isOpen == false) {
    Factory.sol, 178: reflist[address(_template)][i][_references[i]] == true ||
    Factory.sol, 179: reflist[address(_template)][i][address(0)] == true,
    Factory.sol, 197: ) == false
    Factory.sol, 204: if (templates[address(_template)].allowDuplicate == false) {
    IndexTemplate.sol, 132: initialized == false &&
    IndexTemplate.sol, 165: require(locked == false && paused == false, "ERROR: DEPOSIT_DISABLED");
    IndexTemplate.sol, 217: require(locked == false, "ERROR: WITHDRAWAL_PENDING");
    IndexTemplate.sol, 365: IPoolTemplate(_pool).paused() == true
    IndexTemplate.sol, 464: IPoolTemplate(poolList[i]).paused() == false,
    PoolTemplate.sol, 184: initialized == false &&
    PoolTemplate.sol, 234: marketStatus == MarketStatus.Trading && paused == false,
    PoolTemplate.sol, 260: marketStatus == MarketStatus.Trading && paused == false,
    PoolTemplate.sol, 354: insurances[_id].status == true &&
    PoolTemplate.sol, 360: insurances[_id].status == false;
    PoolTemplate.sol, 388: if (_index.exist == false) {
    PoolTemplate.sol, 491: require(paused == false, "ERROR: INSURE_MARKET_PAUSED");
    PoolTemplate.sol, 550: require(_insurance.status == true, "ERROR: INSURANCE_NOT_ACTIVE");
    PoolTemplate.sol, 612: insurance.status == true,
    PoolTemplate.sol, 664: require(paused == false, "ERROR: UNABLE_TO_APPLY");

Handle

robee

Vulnerability details

The following functions could be set private to save gas and improve code quality: _beforeTokenTransfer, CDSTemplate.sol _createClone, Factory.sol _adjustAlloc, IndexTemplate.sol _beforeTokenTransfer, IndexTemplate.sol _accruedPremiums, IndexTemplate.sol initializeToken, InsureDAOERC20.sol _transfer, InsureDAOERC20.sol _mint, InsureDAOERC20.sol _burn, InsureDAOERC20.sol _approve, InsureDAOERC20.sol _beforeTokenTransfer, InsureDAOERC20.sol _afterTokenTransfer, InsureDAOERC20.sol _depositFrom, PoolTemplate.sol _beforeTokenTransfer, PoolTemplate.sol _divCeil, PoolTemplate.sol _sub, PoolTemplate.sol sqrt, BondingPremium.sol _withdrawAttribution, Vault.sol _unutilize, Vault.sol

Handle

robee

Vulnerability details

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base. The format is <solidity file>, <unused storage variable name>:

    IndexTemplate.sol, pendingEnd
    BondingPremium.sol, ADJUSTER

Handle

robee

Vulnerability details

In for loops you initialize the index to start from 0, but it already initialized to 0 in default and this assignment cost gas. It is more clear and gas efficient to declare without assigning 0 and will have the same meaning:

    IndexTemplate.sol, 348
    IndexTemplate.sol, 381
    IndexTemplate.sol, 462
    IndexTemplate.sol, 655
    PoolTemplate.sol, 343
    PoolTemplate.sol, 671
    PoolTemplate.sol, 703
    Vault.sol, 109

Handle

robee

Vulnerability details

Caching the array length is more gas efficient. This is because access to a local variable in solidity is more efficient than query storage / calldata / memory We recommend to change from: for (uint256 i=0; i<array.length; i++) { ... } to: uint len = array.length for (uint256 i=0; i<len; i++) { ... } These functions use not using prefix increments (++x) or not using the unchecked keyword:

    IndexTemplate.sol, poolList, 655
    PoolTemplate.sol, _ids, 343
    PoolTemplate.sol, indexList, 671
    PoolTemplate.sol, indexList, 703

Handle

robee

Vulnerability details

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

    CDSTemplate.sol: parameters.getLockup is read twice in withdraw
    Factory.sol: registry is read twice in createMarket
    IndexTemplate.sol: totalAllocPoint is read twice in set
    IndexTemplate.sol: MAGIC_SCALE_1E6 is read twice in withdrawable
    PoolTemplate.sol: parameters.getLockup is read twice in withdraw
    PoolTemplate.sol: lockedAmount is read twice in utilizationRate
    PoolTemplate.sol: MAGIC_SCALE_1E6 is read twice in allocateCredit
    PoolTemplate.sol: MAGIC_SCALE_1E6 is read twice in withdrawCredit
    PoolTemplate.sol: MAGIC_SCALE_1E6 is read twice in insure
    PoolTemplate.sol: MAGIC_SCALE_1E6 is read twice in resume
    BondingPremium.sol: k is read twice in getCurrentPremiumRate
    BondingPremium.sol: k is read twice in getPremiumRate
    BondingPremium.sol: c is read twice in getPremiumRate
    BondingPremium.sol: b is read twice in getCurrentPremiumRate
    BondingPremium.sol: b is read twice in getPremiumRate
    BondingPremium.sol: T_1 is read twice in getCurrentPremiumRate
    BondingPremium.sol: T_1 is read twice in getPremiumRate
    BondingPremium.sol: BASE is read twice in getCurrentPremiumRate
    BondingPremium.sol: BASE is read twice in getPremiumRate
    BondingPremium.sol: BASE_x2 is read twice in getCurrentPremiumRate
    BondingPremium.sol: BASE_x2 is read twice in getPremiumRate
    Vault.sol: token is read twice in repayDebt
    Vault.sol: token is read twice in utilize
    Vault.sol: token is read twice in withdrawRedundant
    Vault.sol: totalAttributions is read twice in attributionValue
    Vault.sol: balance is read twice in valueAll
    Vault.sol: balance is read twice in withdrawRedundant

Handle

robee

Vulnerability details

Due to how the EVM natively works on 256 numbers, using a 8 bit number here introduces additional costs as the EVM has to properly enforce the limits of this smaller type. See the warning at this link: https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html#layout-of-state-variables-in-storage We recommend to use uint256 for the index in every for loop instead using uint8:

    Vault.sol, uint128 i, 109

Handle

robee

Vulnerability details

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    The function totalLiquidity in CDSTemplate.sol could be set external
    The function valueOfUnderlying in CDSTemplate.sol could be set external
    The function createMarket in Factory.sol could be set external
    The function totalLiquidity in IndexTemplate.sol could be set external
    The function set in IndexTemplate.sol could be set external
    The function leverage in IndexTemplate.sol could be set external
    The function withdrawable in IndexTemplate.sol could be set external
    The function valueOfUnderlying in IndexTemplate.sol could be set external
    The function deposit in IndexTemplate.sol could be set external
    The function adjustAlloc in IndexTemplate.sol could be set external
    The function allowance in InsureDAOERC20.sol could be set external
    The function decimals in InsureDAOERC20.sol could be set external
    The function totalSupply in InsureDAOERC20.sol could be set external
    The function name in InsureDAOERC20.sol could be set external
    The function transfer in InsureDAOERC20.sol could be set external
    The function increaseAllowance in InsureDAOERC20.sol could be set external
    The function transferFrom in InsureDAOERC20.sol could be set external
    The function decreaseAllowance in InsureDAOERC20.sol could be set external
    The function balanceOf in InsureDAOERC20.sol could be set external
    The function symbol in InsureDAOERC20.sol could be set external
    The function approve in InsureDAOERC20.sol could be set external
    The function getOwner in Parameters.sol could be set external
    The function utilizationRate in PoolTemplate.sol could be set external
    The function totalLiquidity in PoolTemplate.sol could be set external
    The function availableBalance in PoolTemplate.sol could be set external
    The function getPremium in PoolTemplate.sol could be set external
    The function valueOfUnderlying in PoolTemplate.sol could be set external
    The function unlock in PoolTemplate.sol could be set external
    The function originalLiquidity in PoolTemplate.sol could be set external
    The function deposit in PoolTemplate.sol could be set external
    The function allocatedCredit in PoolTemplate.sol could be set external
    The function getPremiumRate in BondingPremium.sol could be set external
    The function getCurrentPremiumRate in BondingPremium.sol could be set external
    The function getPricePerFullShare in Vault.sol could be set external
    The function valueAll in Vault.sol could be set external
    The function setController in Vault.sol could be set external
    The function underlyingValue in Vault.sol could be set external

Handle

robee

Vulnerability details

In the following files there are state variables that could be set immutable to save gas. The list of format <solidity file>, <state variable name that could be immutable>:

    registry in Factory.sol
    ownership in Factory.sol
    ownership in Parameters.sol
    ownership in BondingPremium.sol
    ownership in Registry.sol

Handle

robee

Vulnerability details

Prefix increments are cheaper than postfix increments. Further more, using unchecked {++x} is even more gas efficient, and the gas saving accumulates every iteration and can make a real change There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for (uint256 i = 0; i < numIterations; ++i)). But increments perform overflow checks that are not necessary in this case. These functions use not using prefix increments (++x) or not using the unchecked keyword:

    change to prefix increment and unchecked: IndexTemplate.sol, i, 348
    change to prefix increment and unchecked: IndexTemplate.sol, i, 381
    change to prefix increment and unchecked: IndexTemplate.sol, i, 462
    change to prefix increment and unchecked: IndexTemplate.sol, i, 655
    change to prefix increment and unchecked: PoolTemplate.sol, i, 343
    change to prefix increment and unchecked: PoolTemplate.sol, i, 671
    change to prefix increment and unchecked: PoolTemplate.sol, i, 703
    change to prefix increment and unchecked: Vault.sol, i, 109

Handle

robee

Vulnerability details

Use bytes32 instead of string to save gas whenever possible. String is a dynamic data structure and therefore is more gas consuming then bytes32. You could use bytes32 instead of string in the following places: List format: <solidity file> <line number> <actual code line>

    CDSTemplate.sol, 108, string memory _name = "InsureDAO-CDS"; 
    CDSTemplate.sol, 109, string memory _symbol = "iCDS"; 
    IndexTemplate.sol, 141, string memory _name = "InsureDAO-Index"; 
    IndexTemplate.sol, 142, string memory _symbol = "iIndex"; 
    InsureDAOERC20.sol, 16, string private _name = "InsureDAO LP Token"; 
    InsureDAOERC20.sol, 17, string private _symbol = "iLP";