InsureDAO contest - Dravee's results

Handle

Dravee

Vulnerability details

Impact

As mistakes happen, address(0) checks should be made to avoid having to redeploy contracts

Proof of Concept

Here, registry can't be updated after deployment: https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Factory.sol#L82

Tools Used

VS Code

Recommended Mitigation Steps

Add the address(0) check

Handle

Dravee

Vulnerability details

require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking (properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix).

Here, I believe the assert should be a require or a revert:

contracts\Vault.sol:168:            assert(available() >= _amount);

As the Solidity version is > 0.8.0, the remaining gas would still be refunded in case of failure, so I'll just flag this issue as Non-critical

Handle

Dravee

Vulnerability details

In Parameters.sol:setFeeRate, the fee rate isn't bounded. It could get troublesome for users in the case of malicious/faulty governor contract :

172:     /**
173:      * @notice set the contract address of fee model
174:      * @param _address address to set the fee model
175:      * @param _target fee rate
176:      */
177:     function setFeeRate(address _address, uint256 _target)
178:         external
179:         override
180:         onlyOwner
181:     {
182:         _fee[_address] = _target;
183:         emit FeeRateSet(_address, _target);
184:     }

Recommended Mitigation Steps

Set a max fee

Handle

Dravee

Vulnerability details

Impact

A division by 0 could occur

Proof of Concept

While at some places, a check is made to make sure that totalSupply() > 0, it's not consistently the case, such as in the following places:

contracts\CDSTemplate.sol:235:        _retVal = (vault.attributionValue(crowdPool) * _amount) / totalSupply();
contracts\CDSTemplate.sol:318:                _balance * vault.attributionValue(crowdPool) / totalSupply();
contracts\IndexTemplate.sol:216:        _retVal = (_liquidty * _amount) / totalSupply();
contracts\IndexTemplate.sol:530:            return (_balance * totalLiquidity()) / totalSupply();
contracts\PoolTemplate.sol:768:            return (_balance * originalLiquidity()) / totalSupply();

At the following places, the check is indeed made:

contracts\IndexTemplate.sol:514:            return (totalLiquidity() * MAGIC_SCALE_1E6) / totalSupply();
contracts\PoolTemplate.sol:747:            return (originalLiquidity() * MAGIC_SCALE_1E6) / totalSupply();

Tools Used

VS Code

Recommended Mitigation Steps

If this check is at least made at some places, this means that totalSupply() can indeed take a value of 0. Therefore, a check should always be made to prevent the div by 0

Handle

Dravee

Vulnerability details

The spec doesn't match with the comments in the code here:

Code: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Factory.sol#L90-L91 Spec: https://insuredao.gitbook.io/developers/market/factory#approvetemplate

Here, the spec doesn't mention _isOpen and seem to confuse the _approval description with what _isOpen should be.

Tools Used

VS Code

Recommended Mitigation Steps

My guess is that the spec should be corrected

Handle

Dravee

Vulnerability details

The spec says the function should be called approveCondition() instead of setCondition: https://insuredao.gitbook.io/developers/market/factory#approvecondition

While this might still be understood nonetheless as setCondition is also mentioned, the spec says that the parameter _slot is the index of the reference array, whereas the code comment says it's the index within condition array: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Factory.sol#L133

Tools Used

VS Code

Recommended Mitigation Steps

My guess is that the spec should be corrected

Handle

Dravee

Vulnerability details

Impact

Division by 0 or functionally incorrect targetLev

POC

A division by targetLev is made here : https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L306 and targetLev can be set to 0 : https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L575

Tools Used

VS Code

Recommended Mitigation Steps

Either make a check on targetLev before setting it here: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L575 or make a check before the division here: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L306

Handle

Dravee

Vulnerability details

Impact

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Parameters.sol#L177-L184

Tools Used

VS Code

Recommended Mitigation Steps

Add a timelock to setter functions of key/critical variables.

Handle

Dravee

Vulnerability details

Impact

It's possible to save gas by optimizing the checks in conditional statements (if, else if and else). This would save a few opcodes and avoid redundant checks.

Proof of Concept

In CDSTemplate.sol:deposit(), the code is as follows:

140:         if (_supply > 0 && _liquidity > 0) { 
141:             _mintAmount = (_amount * _supply) / _liquidity;
142:         } else if (_supply > 0 && _liquidity == 0) {
143:             //when vault lose all underwritten asset = 
144:             _mintAmount = _amount * _supply; //dilute LP token value af. Start CDS again.
145:         } else {
146:             //when _supply == 0,
147:             _mintAmount = _amount;
148:         }

The conditions checks can be optimized with the following (read the @audit-info comments for futher information):

        if (_supply == 0) { 
            _mintAmount = _amount;
        } else if (_liquidity == 0) { // @audit-info : implicit _supply > 0 as above condition is false
            //when vault lose all underwritten asset = 
            _mintAmount = _amount * _supply; //dilute LP token value af. Start CDS again.
        } else { // @audit-info : implicit _supply > 0 and _liquidity > 0 as both the previous conditions are false
            _mintAmount = (_amount * _supply) / _liquidity;
        }

Tools Used

VS Code

Recommended Mitigation Steps

Compact conditions in mentioned logic statements

Handle

Dravee

Vulnerability details

Impact

Increased gas cost as SSTOREs are very expensive

Proof of Concept

The code is as follows :

094:     function approveTemplate(
095:         IUniversalMarket _template,
096:         bool _approval,
097:         bool _isOpen,
098:         bool _duplicate
099:     ) external override onlyOwner {
100:         require(address(_template) != address(0));
101:         templates[address(_template)].approval = _approval; //@audit-info SSTORE
102:         templates[address(_template)].isOpen = _isOpen; //@audit-info SSTORE
103:         templates[address(_template)].allowDuplicate = _duplicate; //@audit-info SSTORE
104:         emit TemplateApproval(_template, _approval, _isOpen, _duplicate);
105:     }

As we can see, it's making 3 SSTORE operations, one for each boolean. The code could be optimized as follows to save gas :

    function approveTemplate(
        IUniversalMarket _template,
        bool _approval,
        bool _isOpen,
        bool _duplicate
    ) external override onlyOwner {
        require(address(_template) != address(0));
        Template memory approvedTemplate = new Template(_isOpen, _approval, _duplicate);
        templates[address(_template)] = approvedTemplate; //@audit-info only one SSTORE
        emit TemplateApproval(_template, _approval, _isOpen, _duplicate);
    }

Tools Used

VS Code

Recommended Mitigation Steps

Use a memory Template struct and write in storage only once

Handle

Dravee

Vulnerability details

Impact

Reducing from public to private will save gas

Proof of Concept

PremiumModels\BondingPremium.sol:26:    //constants
PremiumModels\BondingPremium.sol:27:    uint256 public constant DECIMAL = uint256(1e6); //Decimals of USDC
PremiumModels\BondingPremium.sol:28:    uint256 public constant BASE = uint256(1e6); //bonding curve graph takes 1e6 as 100.0000%
PremiumModels\BondingPremium.sol:29:    uint256 public constant BASE_x2 = uint256(1e12); //BASE^2
PremiumModels\BondingPremium.sol:30:    uint256 public constant ADJUSTER = uint256(10); //adjuster of 1e6 to 1e5 (100.0000% to 100.000%)
CDSTemplate.sol:55:    uint256 public constant MAGIC_SCALE_1E6 = 1e6; //internal multiplication scale 1e6 to reduce decimal truncation
IndexTemplate.sol:95:    uint256 public constant MAGIC_SCALE_1E6 = 1e6; //internal multiplication scale 1e6 to reduce decimal truncation
PoolTemplate.sol:146:    uint256 public constant MAGIC_SCALE_1E6 = 1e6; //internal multiplication scale 1e6 to reduce decimal truncation
Vault.sol:38:    uint256 public constant MAGIC_SCALE_1E6 = 1e6; //internal multiplication scale 1e6 to reduce decimal truncation

Tools Used

VS Code

Recommended Mitigation Steps

Theses constants can simply be read from the verified contract, i.e., it is unnecessary to expose it with a public function. Also, constants having "1E6" in their name aren't even "nice to have public constants", as their value is obvious.

Handle

Dravee

Vulnerability details

Impact

It's possible to save gas by optimizing the checks in conditional statements (if, else if and else). This would save a few opcodes and avoid redundant checks.

Proof of Concept

In IndexTemplate.sol:deposit(), the code is as follows:

172:         if (_supply > 0 && _totalLiquidity > 0) {  
173:             _mintAmount = (_amount * _supply) / _totalLiquidity;
174:         } else if (_supply > 0 && _totalLiquidity == 0) {
175:             //when
176:             _mintAmount = _amount * _supply;
177:         } else {
178:             _mintAmount = _amount;
179:         }

The conditions checks can be optimized with the following (read the @audit-info comments for further information):

      if (_supply == 0) {
          _mintAmount = _amount;
      } else if (_totalLiquidity == 0) { // @audit-info : implicit _supply > 0 as above condition is false
          _mintAmount = _amount * _supply;
      } else { // @audit-info : implicit _supply > 0 and _totalLiquidity > 0 as both the previous conditions are false
          _mintAmount = (_amount * _supply) / _totalLiquidity;
      }

Tools Used

VS Code

Recommended Mitigation Steps

Compact conditions in mentioned logic statements

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive

Proof of Concept

Here, totalLiquidity() is loaded twice from storage

491:     function leverage() public view returns (uint256 _rate) {
492:         //check current leverage rate
493:         if (totalLiquidity() > 0) {
494:             return (totalAllocatedCredit * MAGIC_SCALE_1E6) / totalLiquidity();
495:         } else {
496:             return 0;
497:         }
498:     }

Tools Used

VS Code

Recommended Mitigation Steps

Cache totalLiquidity() in a variable

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive

Proof of Concept

Here, _fee[_target] can be loaded twice from storage:

271:     function getFeeRate(address _target)
272:         external
273:         view
274:         override
275:         returns (uint256)
276:     {
277:         if (_fee[_target] == 0) {
278:             return _fee[address(0)];
279:         } else {
280:             return _fee[_target];
281:         }
282:     }

Tools Used

VS Code

Recommended Mitigation Steps

Cache the storage reading in a memory variable

Handle

Dravee

Vulnerability details

Impact

SLOADs are expensive

Proof of Concept

Here, attributions[_target] can be loaded twice from storage:

Vault.sol
400:     function underlyingValue(address _target)
401:         public
402:         view
403:         override
404:         returns (uint256)
405:     {
406:         if (attributions[_target] > 0) {
407:             return (valueAll() * attributions[_target]) / totalAttributions;
408:         } else {
409:             return 0;
410:         }
411:     }

Tools Used

VS Code

Recommended Mitigation Steps

Cache the loaded storage value in a memory variable

Handle

Dravee

Vulnerability details

Impact

Checking if the value is 0 before returning 0 is less expensive than returning a calculation that's equal to 0

Proof of Concept

In Vault.sol:underlyingValue(), the code is as follows:

Vault.sol
400:     function underlyingValue(address _target)
401:         public
402:         view
403:         override
404:         returns (uint256)
405:     {
406:         if (attributions[_target] > 0) {
407:             return (valueAll() * attributions[_target]) / totalAttributions;
408:         } else {
409:             return 0;
410:         }
411:     }

It can be optimized as such:

406:         uint256 valueAll = valueAll();
407:         if (valueAll != 0 && attributions[_target] > 0) {
408:             return (valueAll * attributions[_target]) / totalAttributions;
409:         } else {
410:             return 0;
411:         }

Tools Used

VS Code

Recommended Mitigation Steps

Cache the loaded storage value in a memory variable and make the 0 checks to avoid unnecessary calculations if valueAll() == 0

Handle

Dravee

Vulnerability details

Impact

Fixed arrays are less expensive than dynamic arrays and would implicitely add the array.length check

Proof of Concept

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

Tools Used

VS Code

Recommended Mitigation Steps

Make _conditions a fixed array of size 2

Handle

Dravee

Vulnerability details

Impact

Fixed arrays are less expensive than dynamic arrays and would implicitely add the array.length check

Proof of Concept

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

Tools Used

VS Code

Recommended Mitigation Steps

Make _references a fixed array of size 5. I submitted a similar finding for _conditions. While the initialize function is indeed inherited from IUniversalMarket, this is in PoolTemplate that these arrays are the largest. Their bounds should then respectively be 5 and 2.

Handle

Dravee

Vulnerability details

Impact

Increased gas cost (1 MSTORE and 1 MLOAD)

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L552 There's no readability or gas gain from copying incident.payoutNumerator to a variable as it's used only once in the method.

Tools Used

VS Code

Recommended Mitigation Steps

Do not store this data in a variable

Handle

Dravee

Vulnerability details

Impact

Increased gas cost (1 MSTORE and 1 MLOAD)

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L553 There's no readability or gas gain from copying incident.payoutDenominator to a variable as it's used only once in the method.

Tools Used

VS Code

Recommended Mitigation Steps

Do not store this data in a variable

Handle

Dravee

Vulnerability details

Impact

It's possible to save gas by optimizing the checks in conditional statements (if, else if and else). This would save a few opcodes and avoid redundant checks.

Proof of Concept

In PoolTemplate.sol:worth(), the code is as follows:

799:     function worth(uint256 _value) public view returns (uint256 _amount) {
800:         uint256 _supply = totalSupply();
801:         uint256 _originalLiquidity = originalLiquidity();
802:         if (_supply > 0 && _originalLiquidity > 0) {
803:             _amount = (_value * _supply) / _originalLiquidity;
804:         } else if (_supply > 0 && _originalLiquidity == 0) {
805:             _amount = _value * _supply;
806:         } else {
807:             _amount = _value;
808:         }
809:     }

The conditions checks can be optimized with the following (read the @audit-info comments for further information):

    function worth(uint256 _value) public view returns (uint256 _amount) {
        uint256 _supply = totalSupply();
        uint256 _originalLiquidity = originalLiquidity();
        if (_supply == 0) {
            _amount = _value;
        } else if (_originalLiquidity == 0) {
            _amount = _value * _supply;
        } else {
            _amount = (_value * _supply) / _originalLiquidity;
        }
    }

Tools Used

VS Code

Recommended Mitigation Steps

Compact conditions in mentioned logic statements

Handle

Dravee

Vulnerability details

Impact

!= 0 costs less gas compared to > 0 for unsigned integer

Proof of Concept

> 0 is used in the following location(s):

CDSTemplate.sol:100:                bytes(_metaData).length > 0 &&
CDSTemplate.sol:132:        require(_amount > 0, "ERROR: DEPOSIT_ZERO");
CDSTemplate.sol:140:        if (_supply > 0 && _liquidity > 0) {
CDSTemplate.sol:142:        } else if (_supply > 0 && _liquidity == 0) {
CDSTemplate.sol:191:        require(_amount > 0, "ERROR: REQUEST_ZERO");
CDSTemplate.sol:223:        require(_amount > 0, "ERROR: WITHDRAWAL_ZERO");
CDSTemplate.sol:296:        if (totalSupply() > 0) {
Factory.sol:175:        if (_references.length > 0) {
Factory.sol:185:        if (_conditions.length > 0) {
Factory.sol:187:                if (conditionlist[address(_template)][i] > 0) {
IndexTemplate.sol:133:                bytes(_metaData).length > 0 &&
IndexTemplate.sol:166:        require(_amount > 0, "ERROR: DEPOSIT_ZERO");
IndexTemplate.sol:172:        if (_supply > 0 && _totalLiquidity > 0) {
IndexTemplate.sol:174:        } else if (_supply > 0 && _totalLiquidity == 0) {
IndexTemplate.sol:199:        require(_amount > 0, "ERROR: REQUEST_ZERO");
IndexTemplate.sol:231:        require(_amount > 0, "ERROR: WITHDRAWAL_ZERO");
IndexTemplate.sol:246:        if (_liquidityAfter > 0) {
IndexTemplate.sol:274:        if(_totalLiquidity > 0){
IndexTemplate.sol:283:                if (_allocPoint > 0) {
IndexTemplate.sol:391:                if (_current > _target && _available != 0) {
IndexTemplate.sol:427:            allocPoints[msg.sender] > 0,
IndexTemplate.sol:477:        require(allocPoints[msg.sender] > 0);
IndexTemplate.sol:493:        if (totalLiquidity() > 0) {
IndexTemplate.sol:513:        if (totalSupply() > 0) {
IndexTemplate.sol:612:        if (totalAllocPoint > 0) {
IndexTemplate.sol:656:            if (allocPoints[poolList[i]] > 0) {
InsureDAOERC20.sol:302:        require(accountBalance >= amount, "ERC20: burn amount exceeds balance");
Parameters.sol:31:    mapping(address => uint256) private _fee; //fee rate in 1e6 (100% = 1e6)
PoolTemplate.sol:185:                bytes(_metaData).length > 0 &&
PoolTemplate.sol:218:        if (_conditions[1] > 0) {
PoolTemplate.sol:237:        require(_amount > 0, "ERROR: DEPOSIT_ZERO");
PoolTemplate.sol:263:        require(_amount > 0, "ERROR: DEPOSIT_ZERO");
PoolTemplate.sol:282:        require(_amount > 0, "ERROR: REQUEST_ZERO");
PoolTemplate.sol:321:        require(_amount > 0, "ERROR: WITHDRAWAL_ZERO");
PoolTemplate.sol:391:        } else if (_index.credit > 0) {
PoolTemplate.sol:396:            if (_pending > 0) {
PoolTemplate.sol:401:        if (_credit > 0) {
PoolTemplate.sol:437:        if (_credit > 0) {
PoolTemplate.sol:444:        if (_pending > 0) {
PoolTemplate.sol:521:        if (_totalCredit > 0) {
PoolTemplate.sol:672:            if (indicies[indexList[i]].credit > 0) {
PoolTemplate.sol:706:            if (_credit > 0) {
PoolTemplate.sol:726:        if (_deductionFromPool > 0) {
PoolTemplate.sol:745:        if (totalSupply() > 0) {
PoolTemplate.sol:802:        if (_supply > 0 && _originalLiquidity > 0) {
PoolTemplate.sol:804:        } else if (_supply > 0 && _originalLiquidity == 0) {
PoolTemplate.sol:835:        if (totalLiquidity() > 0) {
PoolTemplate.sol:847:        if (lockedAmount > 0) {
PoolTemplate.sol:929:        require(b > 0);
Vault.sol:154:            attributions[msg.sender] > 0 &&
Vault.sol:187:            attributions[msg.sender] > 0 &&
Vault.sol:220:            attributions[msg.sender] > 0 &&
Vault.sol:347:        if (_amount > 0) {
Vault.sol:388:        if (totalAttributions > 0 && _attribution > 0) {
Vault.sol:406:        if (attributions[_target] > 0) {
Vault.sol:473:        } else if (IERC20(_token).balanceOf(address(this)) > 0) {

Tools Used

VS Code

Recommended Mitigation Steps

Change > 0 with != 0.

Handle

Dravee

Vulnerability details

Impact

Checking if the value is 0 before returning 0 is less expensive than returning a calculation that's equal to 0

Proof of Concept

In PoolTemplate.sol:rate(), the code is as follows:

File: PoolTemplate.sol
744:     function rate() external view returns (uint256) {
745:         if (totalSupply() > 0) {
746:             return (originalLiquidity() * MAGIC_SCALE_1E6) / totalSupply();
747:         } else {
748:             return 0;
749:         }
750:     }

It can be optimized as such:

744:     function rate() external view returns (uint256) {
745:         uint256 originalLiquidity = originalLiquidity();
746:         if (originalLiquidity != 0 && totalSupply() > 0) {
747:             return (originalLiquidity * MAGIC_SCALE_1E6) / totalSupply();
748:         } else {
749:             return 0;
750:         }
751:     }

Tools Used

VS Code

Recommended Mitigation Steps

Cache the loaded storage value in a memory variable and make the 0 checks to avoid unnecessary calculations if originalLiquidity() == 0

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

Solidity version 0.8.* already implements overflow and underflow checks by default. Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8.* overflow checks) is therefore redundant.

Instances include:

mocks\ERC20.sol:4:import "@openzeppelin/contracts/utils/math/SafeMath.sol";
mocks\ERC20.sol:30:    using SafeMath for uint256;
mocks\TestPremiumModel.sol:3:import "@openzeppelin/contracts/utils/math/SafeMath.sol";
mocks\TestPremiumModel.sol:7:    using SafeMath for uint256;
PremiumModels\BondingPremium.sol:10:import "@openzeppelin/contracts/utils/math/SafeMath.sol";

Tools Used

VS Code

Recommended Mitigation Steps

Use the built-in checks instead of SafeMath and remove SafeMath from the dependencies

Handle

Dravee

Vulnerability details

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

Instances include:

Factory.sol:176:            for (uint256 i = 0; i < _references.length; i++) {
Factory.sol:186:            for (uint256 i = 0; i < _conditions.length; i++) {
IndexTemplate.sol:280:            for (uint256 i = 0; i < _length; i++) {
IndexTemplate.sol:348:        for (uint256 i = 0; i < _length; i++) {
IndexTemplate.sol:381:        for (uint256 i = 0; i < _length; i++) {
IndexTemplate.sol:462:        for (uint256 i = 0; i < _poolLength; i++) {
IndexTemplate.sol:655:        for (uint256 i = 0; i < poolList.length; i++) {
PoolTemplate.sol:343:        for (uint256 i = 0; i < _ids.length; i++) {
PoolTemplate.sol:671:        for (uint256 i = 0; i < indexList.length; i++) {
PoolTemplate.sol:703:        for (uint256 i = 0; i < indexList.length; i++) {
Vault.sol:109:        for (uint128 i = 0; i < 2; i++) {

Tools Used

Manual Analysis

Recommended Mitigation Steps

Remove explicit initialization for default values.

Handle

Dravee

Vulnerability details

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

Factory.sol:176:            for (uint256 i = 0; i < _references.length; i++) {
Factory.sol:186:            for (uint256 i = 0; i < _conditions.length; i++) {
IndexTemplate.sol:655:        for (uint256 i = 0; i < poolList.length; i++) {
PoolTemplate.sol:343:        for (uint256 i = 0; i < _ids.length; i++) {
PoolTemplate.sol:671:        for (uint256 i = 0; i < indexList.length; i++) {
PoolTemplate.sol:703:        for (uint256 i = 0; i < indexList.length; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Store the array's length in a variable before the for-loop, and use it instead.

Handle

Dravee

Vulnerability details

Impact

Due to how the EVM natively works on 256 bit numbers, using a 8 bit number in for-loops 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 :

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size. It is only beneficial to use reduced-size arguments if you are dealing with storage values because the compiler will pack multiple elements into one storage slot, and thus, combine multiple reads or writes into a single operation. When dealing with function arguments or memory values, there is no inherent benefit because the compiler does not pack these values.

Proof of Concept

Vault.sol:109:        for (uint128 i = 0; i < 2; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Use uint256 as a counter in for-loops.

Handle

Dravee

Vulnerability details

Impact

Using both named returns and a return statement isn't necessary. Removing unused named return variables can reduce gas usage and improve code clarity. To save gas and improve code quality: consider using only one of those.

Proof of Concept

Instances include:

CDSTemplate.sol:287:    function totalLiquidity() public view returns (uint256 _balance) {
IndexTemplate.sol:491:    function leverage() public view returns (uint256 _rate) {
IndexTemplate.sol:504:    function totalLiquidity() public view returns (uint256 _balance) {
PoolTemplate.sol:628:        returns (uint256 premium)
PoolTemplate.sol:833:        returns (uint256 _balance)
PoolTemplate.sol:846:    function utilizationRate() public view override returns (uint256 _rate) {
PoolTemplate.sol:858:    function totalLiquidity() public view override returns (uint256 _balance) {
PoolTemplate.sol:866:    function originalLiquidity() public view returns (uint256 _balance) {

Tools Used

VS Code

Recommended Mitigation Steps

Remove the unused named returns

Handle

Dravee

Vulnerability details

Impact

On external functions, when using the memory keyword with a function argument, what's happening is that a memory acts as an intermediate.

Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

As an extract from https://ethereum.stackexchange.com/questions/74442/when-should-i-use-calldata-and-when-should-i-use-memory :

memory and calldata (as well as storage) are keywords that define the data area where a variable is stored. To answer your question directly, memory should be used when declaring variables (both function parameters as well as inside the logic of a function) that you want stored in memory (temporary), and calldata must be used when declaring an external function's dynamic parameters. The easiest way to think about the difference is that calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Proof of Concept

Vault.sol:92:        address[2] memory _beneficiaries,
Vault.sol:93:        uint256[2] memory _shares

Tools Used

VS Code

Recommended Mitigation Steps

Use calldata instead of memory for external functions where the function argument is read-only.

Handle

Dravee

Vulnerability details

Impact

Increased gas cost.

Proof of Concept

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers.

When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation, or the operation doesn't depend on user input), some gas can be saved by using an unchecked block.

https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

These lines are the obvious ones that can't underflow or overflow (operations on constants or checks already made before the operations with require statements or if statements):

PremiumModels\BondingPremium.sol:47:        T_1 = 1000000 * DECIMAL;
PremiumModels\BondingPremium.sol:130:        uint256 u1 = BASE - ((_lockedAmount * BASE) / _totalLiquidity); //util rate before. 1000000 = 100.000%
PremiumModels\BondingPremium.sol:132:            (((_lockedAmount + _amount) * BASE) / _totalLiquidity); //util rate after. 1000000 = 100.000%
IndexTemplate.sol:292:                        uint256 _lockedCredit = _allocated - _availableBalance;
PoolTemplate.sol:942:            return a - b;
IndexTemplate.sol:308:                    _retVal = _totalLiquidity - _necessaryAmount;
IndexTemplate.sol:393:                    uint256 _decrease = _current - _target;
IndexTemplate.sol:399:                    uint256 _allocate = _target - _current;
IndexTemplate.sol:441:                _shortage = _amount - _value;
InsureDAOERC20.sol:255:        _balances[sender] = senderBalance - amount;
InsureDAOERC20.sol:303:        _balances[account] = accountBalance - amount;
Vault.sol:165:            uint256 _shortage = _amount - available();
Vault.sol:310:            uint256 _shortage = _retVal - available();

Tools Used

VS Code

Recommended Mitigation Steps

Uncheck arithmetic operations when the risk of underflow or overflow is already contained.

Handle

Dravee

Vulnerability details

Impact

Increased gas cost (1 slot)

Proof of Concept

IndexTemplate.pendingEnd (contracts/IndexTemplate.sol#62) should be deleted as it's never used by the contract

Tools Used

Slither

Recommended Mitigation Steps

Delete the variable IndexTemplate.pendingEnd

Handle

Dravee

Vulnerability details

Impact

Repetitive and expensive SSTORE opcode operations inside loops

Proof of Concept

	totalAllocatedCredit -= _available (contracts/IndexTemplate.sol#368)
	totalAllocatedCredit -= _decrease (contracts/IndexTemplate.sol#395)
	totalAllocatedCredit += _allocate (contracts/IndexTemplate.sol#401)

Tools Used

Slither

Recommended Mitigation Steps

Create a memory variable which will be used to compute a _totalAllocatedCredit that will get added to totalAllocatedCredit storage variable outside the loop. As an idea, you could create 1 such int variable and use it's value after the for-loop, or you could create 2 uint variables where 1 would store the _totalDecrease and 1 would store the _totalAllocate, and respectively substract and add them.

Handle

Dravee

Vulnerability details

Impact

Public functions that are never called by the contract should be declared external to save gas.

Proof of Concept

	- CDSTemplate.totalLiquidity() (contracts/CDSTemplate.sol#287-289)
	- CDSTemplate.valueOfUnderlying(address) (contracts/CDSTemplate.sol#310-318)
	- IndexTemplate.deposit(uint256) (contracts/IndexTemplate.sol#164-190)
	- IndexTemplate.leverage() (contracts/IndexTemplate.sol#491-498)
	- IndexTemplate.valueOfUnderlying(address) (contracts/IndexTemplate.sol#525-532)
	- IndexTemplate.set(uint256,address,uint256) (contracts/IndexTemplate.sol#586-623)
	- InsureDAOERC20.name() (contracts/InsureDAOERC20.sol#42-44)
	- InsureDAOERC20.symbol() (contracts/InsureDAOERC20.sol#50-52)
	- InsureDAOERC20.decimals() (contracts/InsureDAOERC20.sol#67-69)
	- InsureDAOERC20.transfer(address,uint256) (contracts/InsureDAOERC20.sol#99-107)
	- InsureDAOERC20.allowance(address,address) (contracts/InsureDAOERC20.sol#112-120)
	- InsureDAOERC20.approve(address,uint256) (contracts/InsureDAOERC20.sol#129-137)
	- InsureDAOERC20.transferFrom(address,address,uint256) (contracts/InsureDAOERC20.sol#152-168)
	- InsureDAOERC20.increaseAllowance(address,uint256) (contracts/InsureDAOERC20.sol#182-193)
	- InsureDAOERC20.decreaseAllowance(address,uint256) (contracts/InsureDAOERC20.sol#209-223)
	- Parameters.getOwner() (contracts/Parameters.sol#218-220)
	- PoolTemplate.deposit(uint256) (contracts/PoolTemplate.sol#232-247)
	- IPoolTemplate.valueOfUnderlying(address) (contracts/interfaces/IPoolTemplate.sol#27-31)	
	- PoolTemplate.valueOfUnderlying(address) (contracts/PoolTemplate.sol#757-769)
	- PoolTemplate.allocatedCredit(address) (contracts/PoolTemplate.sol#816-823)
	- IPoolTemplate.utilizationRate() (contracts/interfaces/IPoolTemplate.sol#22)
	- PoolTemplate.utilizationRate() (contracts/PoolTemplate.sol#846-852)
	- BondingPremium.getCurrentPremiumRate(uint256,uint256) (contracts/PremiumModels/BondingPremium.sol#61-102)
	- Vault.getPricePerFullShare() (contracts/Vault.sol#448-450)
	- Vault.setController(address) (contracts/Vault.sol#485-496)

Tools Used

Slither

Recommended Mitigation Steps

Change the visibility to external

Handle

Dravee

Vulnerability details

Impact

The compiler won't reserve a storage slot for immutable variables

Proof of Concept

The following variables are initialized in the contract's constructor and can't get updated after:

Factory.sol:registry
Factory.sol:ownership
Parameters:ownership
BondingPremium:ownership
Registry:ownership
Vault:ownership

Tools Used

VS Code

Recommended Mitigation Steps

Make these variables immutable

Handle

Dravee

Vulnerability details

Impact

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Proof of Concept

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

Factory.sol:176:            for (uint256 i = 0; i < _references.length; i++) {
Factory.sol:186:            for (uint256 i = 0; i < _conditions.length; i++) {
IndexTemplate.sol:280:            for (uint256 i = 0; i < _length; i++) {
IndexTemplate.sol:348:        for (uint256 i = 0; i < _length; i++) {
IndexTemplate.sol:381:        for (uint256 i = 0; i < _length; i++) {
IndexTemplate.sol:462:        for (uint256 i = 0; i < _poolLength; i++) {
IndexTemplate.sol:655:        for (uint256 i = 0; i < poolList.length; i++) {
PoolTemplate.sol:343:        for (uint256 i = 0; i < _ids.length; i++) {
PoolTemplate.sol:671:        for (uint256 i = 0; i < indexList.length; i++) {
PoolTemplate.sol:703:        for (uint256 i = 0; i < indexList.length; i++) {
Vault.sol:109:        for (uint128 i = 0; i < 2; i++) {

Tools Used

VS Code

Recommended Mitigation Steps

Use ++i instead of i++ to increment the value of an uint variable.

Handle

Dravee

Vulnerability details

Impact

When a contract imports and implements an interface or another contracts, it doesn't need to import the libraries that were already imported there.

Removing these imports will save gas.

Proof of Concept

InsureDAOERC20 imports the following:

5: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
6: import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";

The following contracts inherit InsureDAOERC20 and also make those imports: CDSTemplate, IndexTemplate, PoolTemplate

Tools Used

VS Code

Recommended Mitigation Steps

Remove the unused imports to reduce the size of the contract and save some deployment gas.

Handle

Dravee

Vulnerability details

Impact

Duplicated code, loss of maintainability, increased contract size which leads to increased gas cost

Proof of Concept

The following can be simplified:

260:         if (_available >= _amount) {
261:             _compensated = _amount;
262:             _attributionLoss = vault.transferValue(_amount, msg.sender);
263:             emit Compensated(msg.sender, _amount);
264:         } else {
265:             //when CDS cannot afford, pay as much as possible
266:             _compensated = _available;
267:             _attributionLoss = vault.transferValue(_available, msg.sender);
268:             emit Compensated(msg.sender, _available);
269:         }

to

260:         _compensated = _available >= _amount ? _amount : _available; //when CDS cannot afford, pay as much as possible
261:         _attributionLoss = vault.transferValue(_compensated, msg.sender);
262:         emit Compensated(msg.sender, _compensated);

Tools Used

VS Code

Recommended Mitigation Steps

Apply the refacto and look out for duplicated code

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

The variable T_0 can go through 2 assignments in a row: Here:

75:         uint256 T_0 = _totalLiquidity;
76:         if (T_0 > T_1) {
77:             T_0 = T_1;
78:         }

And here:

134:         uint256 T_0 = _totalLiquidity;
135:         if (T_0 > T_1) {
136:             T_0 = T_1;
137:         }

The code can be optimized as such to save some gas:

        uint256 T_0 = _totalLiquidity > T_1 ? _totalLiquidity : T_1;

Tools Used

VS Code

Recommended Mitigation Steps

Apply the refacto

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

In Factory.sol, the following > 0 checks are redundant with the for-loop condition, because if _references.length == 0 or _conditions.length == 0, the condition uint256 i = 0; i <(_conditions)|(_references).length will never be satisfied and the for-loop won't iterate:

175:         if (_references.length > 0) {
176:             for (uint256 i = 0; i < _references.length; i++) {
177:                 require(
178:                     reflist[address(_template)][i][_references[i]] == true ||
179:                         reflist[address(_template)][i][address(0)] == true,
180:                     "ERROR: UNAUTHORIZED_REFERENCE"
181:                 );
182:             }
183:         }
184: 
185:         if (_conditions.length > 0) {
186:             for (uint256 i = 0; i < _conditions.length; i++) {
187:                 if (conditionlist[address(_template)][i] > 0) {
188:                     _conditions[i] = conditionlist[address(_template)][i];
189:                 }
190:             }
191:         }

Tools Used

VS Code

Recommended Mitigation Steps

Remove these 2 if-statements

Handle

Dravee

Vulnerability details

Impact

The operators “||” and “&&” apply the common short-circuiting rules. This means that in the expression “f(x) || g(y)”, if “f(x)” evaluates to true, “g(y)” will not be evaluated even if it may have side-effects.

Source: https://docs.soliditylang.org/en/v0.5.4/types.html#booleans

Proof of Concept

In IndexTemplate.sol:withdrawable(), there's an if-statement as such:

293:                         if (i == 0 || _availableRate < _lowestAvailableRate) {

Here, the condition i == 0 is always evaluated and is always equal to false when i > 0, meaning here a total of poolList.length - 1 evaluations are always evaluated to false.

It's best to reorder the conditions such as this condition doesn't get evaluated if _availableRate < _lowestAvailableRate is satisfied:

293:                         if (_availableRate < _lowestAvailableRate || i == 0 ) {

Tools Used

VS Code

Recommended Mitigation Steps

Apply the refacto

Handle

Dravee

Vulnerability details

Impact

Increased gas cost

Proof of Concept

In IndexTemplate.sol:_adjustAlloc(), the 3 following conditions are always evaluated:

                //Withdraw or Deposit credit
                if (_current > _target && _available != 0) {
                    //if allocated credit is higher than the target, try to decrease
                    uint256 _decrease = _current - _target;
                    IPoolTemplate(_poolList[i].addr).withdrawCredit(_decrease);
                    totalAllocatedCredit -= _decrease;
                }
                if (_current < _target) {
                    uint256 _allocate = _target - _current;
                    IPoolTemplate(_poolList[i].addr).allocateCredit(_allocate);
                    totalAllocatedCredit += _allocate;
                }
                if (_current == _target) {
                    IPoolTemplate(_poolList[i].addr).allocateCredit(0);
                }

The code can be optimized to save some gas:

                if (_current == _target) {
                    IPoolTemplate(_poolList[i].addr).allocateCredit(0);
                } else if (_current < _target) {
                    uint256 _allocate = _target - _current;
                    IPoolTemplate(_poolList[i].addr).allocateCredit(_allocate);
                    totalAllocatedCredit += _allocate;
                } else if (_current > _target && _available != 0) {
                    //Withdraw or Deposit credit
                    //if allocated credit is higher than the target, try to decrease
                    uint256 _decrease = _current - _target;
                    IPoolTemplate(_poolList[i].addr).withdrawCredit(_decrease);
                    totalAllocatedCredit -= _decrease;
                }

Tools Used

VS Code

Recommended Mitigation Steps

Apply the refacto

Handle

Dravee

Vulnerability details

In IndexTemplate.sol:withdrawable(), the following can be optimized to save gas and avoid a loss of precision, from:

                uint256 _necessaryAmount = _targetLockedCreditScore * totalAllocPoint /  _targetAllocPoint;
                _necessaryAmount = _necessaryAmount *  MAGIC_SCALE_1E6 / targetLev;

to

                uint256 _necessaryAmount = _targetLockedCreditScore * totalAllocPoint *  MAGIC_SCALE_1E6 /  (_targetAllocPoint * targetLev);

Handle

Dravee

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of Concept

Revert strings > 32 bytes are here:

PremiumModels\BondingPremium.sol:35:            "Restricted: caller is not allowed to operate"
PremiumModels\BondingPremium.sol:67:            "ERROR: _lockedAmount > _totalLiquidity"
CDSTemplate.sol:104:            "ERROR: INITIALIZATION_BAD_CONDITIONS"
CDSTemplate.sol:217:            "ERROR: WITHDRAWAL_NO_ACTIVE_REQUEST"
CDSTemplate.sol:221:            "ERROR: WITHDRAWAL_EXCEEDED_REQUEST"
Factory.sol:76:            "Restricted: caller is not allowed to operate"
IndexTemplate.sol:103:            "Restricted: caller is not allowed to operate"
IndexTemplate.sol:137:            "ERROR: INITIALIZATION_BAD_CONDITIONS"
IndexTemplate.sol:225:            "ERROR: WITHDRAWAL_NO_ACTIVE_REQUEST"
IndexTemplate.sol:229:            "ERROR: WITHDRAWAL_EXCEEDED_REQUEST"
IndexTemplate.sol:235:            "ERROR: WITHDRAW_INSUFFICIENT_LIQUIDITY"
IndexTemplate.sol:428:            "ERROR_COMPENSATE_UNAUTHORIZED_CALLER"
InsureDAOERC20.sol:162:            "ERC20: transfer amount exceeds allowance"
InsureDAOERC20.sol:217:            "ERC20: decreased allowance below zero"
InsureDAOERC20.sol:252:            "ERC20: transfer amount exceeds balance"
Ownership.sol:39:            "Restricted: caller is not allowed to operate"
Ownership.sol:47:            "Restricted: caller is not allowed to operate"
Parameters.sol:52:            "Restricted: caller is not allowed to operate"
PoolTemplate.sol:151:            "Restricted: caller is not allowed to operate"
PoolTemplate.sol:192:            "ERROR: INITIALIZATION_BAD_CONDITIONS"
PoolTemplate.sol:315:            "ERROR: WITHDRAWAL_NO_ACTIVE_REQUEST"
PoolTemplate.sol:319:            "ERROR: WITHDRAWAL_EXCEEDED_REQUEST"
PoolTemplate.sol:324:            "ERROR: WITHDRAW_INSUFFICIENT_LIQUIDITY"
PoolTemplate.sol:384:            "ERROR: ALLOCATE_CREDIT_BAD_CONDITIONS"
PoolTemplate.sol:427:            "ERROR: WITHDRAW_CREDIT_BAD_CONDITIONS"
PoolTemplate.sol:478:            "ERROR: INSURE_EXCEEDED_AVAILABLE_BALANCE"
PoolTemplate.sol:613:            "ERROR: INSURANCE_TRANSFER_BAD_CONDITIONS"
Registry.sol:25:            "Restricted: caller is not allowed to operate"
Vault.sol:47:            "Restricted: caller is not allowed to operate"
Vault.sol:156:            "ERROR_WITHDRAW-VALUE_BADCONDITOONS"
Vault.sol:189:            "ERROR_TRANSFER-VALUE_BADCONDITOONS"
Vault.sol:302:            "ERROR_WITHDRAW-ATTRIBUTION_BADCONDITOONS"
Vault.sol:331:            "ERROR_TRANSFER-ATTRIBUTION_BADCONDITOONS" 

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes.