InsureDAO contest - TomFrenchBlockchain's results

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

When transferring any of the market tokens, a check is performed to see if they have a pending withdrawal and reduce it if their balance falls below the requested amount.

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

In the case where a user has no pending withdrawal we then perform an unnecessary check on their balance. We could save an SLOAD by changing it to the below

if (from != address(0)) {
    uint256 reqAmount = withdrawalReq[from].amount
    if (reqAmount > 0){
        uint256 _after = balanceOf(from) - amount;
        if (_after < reqAmount) {
            withdrawalReq[from].amount = _after;
        }
    } 
}

Recommended Mitigation Steps

As above

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

PoolTemplate.sol has an Insurance struct which looks as below:

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

struct Insurance {
    uint256 id;
    uint256 startTime;
    uint256 endTime; 
    uint256 amount; 
    bytes32 target;
    address insured;
    bool status;
}

id, startTime and endTime and potentially amount are all good candidates to be packed to save SSTORES when taking out insurance. A potential (conservative) packing is

struct Insurance {
    uint128 id;
    uint64 startTime;
    uint64 endTime; 
    uint256 amount; 
    bytes32 target;
    address insured;
    bool status;
}

which eliminates 2 SSTORES (40k) gas when taking out insurance.

Recommended Mitigation Steps

Determine the optimal packing for this struct (take into account which fields are read together when paying out for extra savings)

Handle

TomFrenchBlockchain

Vulnerability details

Impact

gas costs

Proof of Concept

In the sqrt function it is known that the while loop will not overflow so it can be safely left unchecked to save gas.

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

    function sqrt(uint256 x) internal pure returns (uint256 y) {
        uint256 z = (x + 1) / 2;
        unchecked {
            y = x;
            while (z < y) {
                y = z;
                z = (x / z + z) / 2;
            }
        }
    }

Recommended Mitigation Steps

wrap entire function body in a unchecked block as above

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

The Withdrawal struct in IndexTemplate.sol contains a timestamp and the amount of tokens which the user requests to withdraw.

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

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

If we make the safe assumption that the user's balance does not exceed 2^192 then we can pack this struct into a single storage slot to save an SLOAD by changing the definition to:

struct Withdrawal {
    uint64 timestamp;
    uint192 amount;
}

Recommended Mitigation Steps

As above

Handle

TomFrenchBlockchain

Vulnerability details

Impact

gas costs

Proof of Concept

Here on L563 we check the market status however we have already done this on L558

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

Recommended Mitigation Steps

Remove redundant check (check other market templates as well)

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

It seems that we always want to get a pool's allocatedCredit and availableBalance together, suggesting that these values are tightly coupled.

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

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

If we're regularly going to be requesting these values together it may be worth considering having a single function in the pool template which returns both of these values. This would save gas costs of performing an extra external call to the pool contract.

Recommended Mitigation Steps

Consider having a function which returns both of these values to avoid repeated calls into the same contract for related info.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

Here we push a new market onto an array in the factory whilst we just added the market to the registry.

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

Recommended Mitigation Steps

This array on the factory seems redundant and so it can be removed.

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Gas costs

Proof of Concept

Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

InsureDAO is using 0.8.7: https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Factory.sol#L8

Updating to the newer version of solc will allow InsureDAO to take advantage of these lower costs for external calls.

Recommended Mitigation Steps

Update to solc 0.8.10 or above

Handle

TomFrenchBlockchain

Vulnerability details

Impact

gas costs

Proof of Concept

We only set this field on construction so it can be safely made immutable.

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

Recommended Mitigation Steps

add immutable keyword

Handle

TomFrenchBlockchain

Vulnerability details

Impact

gas costs

Proof of Concept

Here if the lengths of these arrays are zero we'll fall straight through the for loops so there's no need for the if statements.

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

Recommended Mitigation Steps

Remove if statements