InsureDAO contest - Jujic's results

Handle

Jujic

Vulnerability details

Impact

Consider reviewing all the unused imports and removing them to reduce the size of the contract and thus save some deployment gas.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Factory.sol#L14

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/IndexTemplate.sol#L7

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Parameters.sol#L13

import "hardhat/console.sol";

Tools Used

Remix

Recommended Mitigation Steps

Remove test imports

Handle

Jujic

Vulnerability details

Impact

Some of the variables can be cached to slightly reduce gas usage

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/PoolTemplate.sol#L343

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L406-L407

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L461-L479

function withdrawRedundant(address _token, address _to)
        external
        override
        onlyOwner
    {
        if (
            _token == address(token) &&
            balance < IERC20(token).balanceOf(address(this))
        ) {
            uint256 _redundant = IERC20(token).balanceOf(address(this)) -
                balance;
            IERC20(token).safeTransfer(_to, _redundant);
        } else if (IERC20(_token).balanceOf(address(this)) > 0) {
            IERC20(_token).safeTransfer(
                _to,
                IERC20(_token).balanceOf(address(this))
            );
        }
    }

Tools Used

Remix

Recommended Mitigation Steps

Consider caching those variable for read and make sure write back to storage Example:

bal =  IERC20(_token).balanceOf(address(this);

Handle

Jujic

Vulnerability details

Impact

The use of _msgSender() when there is no implementation of a meta transaction mechanism that uses it, such as EIP-2771, very slightly increases gas consumption.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/InsureDAOERC20.sol#L105

function transfer(address recipient, uint256 amount)
        public
        virtual
        override
        returns (bool)
    {
        _transfer(_msgSender(), recipient, amount);
        return true;
    }

Tools Used

Remix

Recommended Mitigation Steps

Replace _msgSender() with msg.sender if there is no mechanism to support meta-transactions like EIP-2771 implemented.

Handle

Jujic

Vulnerability details

Impact

Checking if _amount != 0 before making the transfer call can save gas by avoiding the external call in such situations.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L201-L206

function borrowValue(uint256 _amount, address _to) external onlyMarket override {
        debts[msg.sender] += _amount;
        totalDebt += _amount;

        IERC20(token).safeTransfer(_to, _amount);
    }

Tools Used

Remix

Recommended Mitigation Steps

Add additional check for non zero _amount.

Handle

Jujic

Vulnerability details

Impact

function rate() external view returns (uint256) {
        if (totalSupply() > 0) {
            return (totalLiquidity() * MAGIC_SCALE_1E6) / totalSupply();
        } else {
            return 0;
        }
    }

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/IndexTemplate.sol#L512-L518

Tools Used

Remix

Recommended Mitigation Steps

Change to:

function rate() external view returns (uint256) {
        if (totalSupply() != 0) {
            return (totalLiquidity() * MAGIC_SCALE_1E6) / totalSupply();
        
    }

Handle

Jujic

Vulnerability details

Impact

Where possible, use equivalent function parameters or local variables in event emits instead of state variables to prevent expensive SLOADs.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/PoolTemplate.sol#L364

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/PoolTemplate.sol#L733

marketStatus = MarketStatus.Trading;
        emit MarketStatusChanged(MarketStatus.Trading);
    }

Tools Used

Remix

Recommended Mitigation Steps

Use equivalent function parameters or local variables in event emits instead of state variables.

Handle

Jujic

Vulnerability details

Impact

Insurance struct in PoolTemplate .sol can be optimized to reduce 2 storage slot

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/PoolTemplate.sol#L127-L128

struct Insurance {
        uint256 id; //each insuance has their own id
        uint256 startTime; //timestamp of starttime
        uint256 endTime; //timestamp of endtime
        uint256 amount; //insured amount
        bytes32 target; //target id in bytes32
        address insured; //the address holds the right to get insured
        bool status; //true if insurance is not expired or redeemed
    }

startTime and endTime store block numbers, and 2^48 is being enough for a very long time.

Tools Used

https://docs.soliditylang.org/en/v0.8.0/internals/layout_in_storage.html?highlight=Structs#layout-of-state-variables-in-storage

Recommended Mitigation Steps

The struct can be changed into:

struct Insurance {
        uint256 id; //each insuance has their own id
        uint48 startTime; //timestamp of starttime
        uint48 endTime; //timestamp of endtime
        address insured; //the address holds the right to get insured
        uint256 amount; //insured amount
        bytes32 target; //target id in bytes32
        bool status; //true if insurance is not expired or redeemed
    }

Handle

Jujic

Vulnerability details

Impact

The constructor is empty. You should remove constructor to save some gas.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/InsureDAOERC20.sol#L21

constructor() {}

Tools Used

Recommended Mitigation Steps

Remove unused constructor

Handle

Jujic

Vulnerability details

Impact

Each function part of contract's external interface is part of the function dispatch, i.e., every time a contract is called, it goes through a switch statement (a set of eq ... JUMPI blocks in EVM) matching the selector of each externally available functions with the chosen function selector (the first 4 bytes of calldata). This means that any unnecessary function that is part of contract's external interface will lead to more gas for (almost) every single function calls to the contract. There are several cases where constants were made public. This is unnecessary; the constants can simply be readfrom the verified contract, i.e., it is unnecessary to expose it with a public function.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L38

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/IndexTemplate.sol#L95

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/PoolTemplate.sol#L146

uint256 public constant MAGIC_SCALE_1E6 = 1e6; //internal multiplication scale 1e6 to reduce decimal truncation

Tools Used

Remix

Recommended Mitigation Steps

Handle

Jujic

Vulnerability details

Impact

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L220

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Factory.sol#L175

require(
            attributions[msg.sender] > 0 &&
                underlyingValue(msg.sender) >= _amount,
            "ERROR_REPAY_DEBT_BADCONDITOONS"
        );

Tools Used

Remix

Recommended Mitigation Steps

The same situation are in other scope contracts where use > 0.

Handle

Jujic

Vulnerability details

Impact

The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.

The same situation are in other scope contracts where loops use.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/PoolTemplate.sol#L343

for (uint256 i = 0; i < _ids.length; i++) 

Tools Used

Remix

Recommended Mitigation Steps

Remove explicit 0 initialization of for loop index variable. Before: for (uint256 i = 0; After for (uint256 i;

Handle

Jujic

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

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/IndexTemplate.sol#L655

function _accruedPremiums() internal view returns (uint256 _totalValue) {
        for (uint256 i = 0; i < poolList.length; i++) {
            if (allocPoints[poolList[i]] > 0) {
                _totalValue =
                    _totalValue +
                    IPoolTemplate(poolList[i]).pendingPremium(address(this));
            }
        }
    }

Tools Used

Remix

Recommended Mitigation Steps

Caching len = poolList.length and using the len instead will save gas.

Handle

Jujic

Vulnerability details

Impact

There is the loop that use uint128 for an index parameter (i). It does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint128 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L109

for (uint128 i = 0; i < 2; i++) {...}

Tools Used

Remix

Recommended Mitigation Steps

Handle

Jujic

Vulnerability details

Impact

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/IndexTemplate.sol#L292

if (_allocated > _availableBalance) {
                        uint256 _availableRate = (_availableBalance *
                            MAGIC_SCALE_1E6) / _allocated;
                        uint256 _lockedCredit = _allocated - _availableBalance;

Tools Used

Remix

Recommended Mitigation Steps

Consider using 'unchecked' where it is safe to do so.

Handle

Jujic

Vulnerability details

Impact

Address token is set on initialization, doesn't change afterwards and should be immutable

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L25

address public override token;

constructor(
        address _token,
        address _registry,
        address _controller,
        address _ownership
    ) {...}

Tools Used

Remix

Recommended Mitigation Steps

Handle

Jujic

Vulnerability details

Impact

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

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Vault.sol#L47

modifier onlyOwner() {
        require(
            ownership.owner() == msg.sender,
            "Restricted: caller is not allowed to operate"
        );
        _;
    }

The same situation are in other scope contracts where require use.

Tools Used

https://planetcalc.com/9029/

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes.

Handle

Jujic

Vulnerability details

Impact

The solc compiler 0.8 will give the in-built overflow/underflow checks which may give gas savings by avoiding expensive SafeMath.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/PremiumModels/BondingPremium.sol#L10

pragma solidity 0.8.7;

import "@openzeppelin/contracts/utils/math/SafeMath.sol";

Tools Used

Remix

Recommended Mitigation Steps

Remove safeMath operations

Handle

Jujic

Vulnerability details

Impact

Here you could use unchecked{++i} to save gas since it is more efficient then i++.

Proof of Concept

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Factory.sol#L176

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Factory.sol#L186

for (uint256 i = 0; i < _references.length; i++) {...}

Tools Used

Remix

Recommended Mitigation Steps

The same situation are in other scope contracts where loops use.