InsureDAO contest - WatchPug's results

Handle

WatchPug

Vulnerability details

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

function initialize(
    string calldata _metaData,
    uint256[] calldata _conditions,
    address[] calldata _references
) external override {
    require(
        initialized == false &&
            bytes(_metaData).length > 0 &&
            _references[0] != address(0) &&
            _references[1] != address(0) &&
            _references[2] != address(0) &&
            _references[3] != address(0) &&
            _references[4] != address(0) &&
            _conditions[0] <= _conditions[1],
        "ERROR: INITIALIZATION_BAD_CONDITIONS"
    );
    initialized = true;

    string memory _name = string(
        abi.encodePacked(
            "InsureDAO-",
            IERC20Metadata(_references[1]).name(),
            "-PoolInsurance"
        )
    );
    string memory _symbol = string(
        abi.encodePacked("i-", IERC20Metadata(_references[1]).symbol())
    );
    uint8 _decimals = IERC20Metadata(_references[0]).decimals();

    initializeToken(_name, _symbol, _decimals);

    registry = IRegistry(_references[2]);
    parameters = IParameters(_references[3]);
    vault = IVault(parameters.getVault(_references[1]));

    metadata = _metaData;

    marketStatus = MarketStatus.Trading;

    if (_conditions[1] > 0) {
        _depositFrom(_conditions[1], _references[4]);
    }
}

Anyone can create a pool from a whitelisted template and can specify certain parameters.

However, the parameters passed to PoolTemplate.sol#initialize() can be used to deposit from a specified address as long as they have approved the Vault contract before.

PoC

• templates[address(_template)].approval == true
• reflist[address(_template)][4][address(0)] = true

1. Alice approved USDC for uint256.max to the Vault contract;
2. Attacker call Factory#createMarket() and set _conditions[1] = 10,000 and _references[4] = Alice's address;

Impact

Users' allowances can be used by anyone.

Recommendation

Change to:

function initialize(
    string calldata _metaData,
    uint256[] calldata _conditions,
    address[] calldata _references
) external override {
    require(
        initialized == false &&
            bytes(_metaData).length > 0 &&
            _references[0] != address(0) &&
            _references[1] != address(0) &&
            _references[2] != address(0) &&
            _references[3] != address(0) &&
            _references[4] != address(0) &&
            _conditions[0] <= _conditions[1],
        "ERROR: INITIALIZATION_BAD_CONDITIONS"
    );
    initialized = true;

    string memory _name = string(
        abi.encodePacked(
            "InsureDAO-",
            IERC20Metadata(_references[1]).name(),
            "-PoolInsurance"
        )
    );
    string memory _symbol = string(
        abi.encodePacked("i-", IERC20Metadata(_references[1]).symbol())
    );
    uint8 _decimals = IERC20Metadata(_references[0]).decimals();

    initializeToken(_name, _symbol, _decimals);

    registry = IRegistry(_references[2]);
    parameters = IParameters(_references[3]);
    vault = IVault(parameters.getVault(_references[1]));

    metadata = _metaData;

    marketStatus = MarketStatus.Trading;

    if (_conditions[1] > 0) {
        _depositFrom(_conditions[1], tx.origin);
    }
}

Handle

WatchPug

Vulnerability details

Based on the context, withdrawRedundant() intends to disallow the owner to withdraw more Vault tokens than the surplus amount.

However, the current implementation is wrong, which allows the Vault owner to drain all the funds.

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

PoC

If the owner's private key is compromised, the attacker can:

When token.balanceOf(address(this)) == balance:

1. Call withdrawRedundant(), withdraw all the funds.

When token.balanceOf(address(this)) > balance:

1. Call withdrawRedundant(), withdraw surplus amount;
2. Call withdrawRedundant(), withdraw all the funds.

When token.balanceOf(address(this)) < balance:

1. Transfer balance - token.balanceOf(address(this)) to the Vault contract;
2. Call withdrawRedundant(), withdraw all the funds.

Recommendation

Change to:

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 (
        _token != address(token) &&
        IERC20(_token).balanceOf(address(this)) > 0
    ) {
        IERC20(_token).safeTransfer(
            _to,
            IERC20(_token).balanceOf(address(this))
        );
    }
}

Handle

WatchPug

Vulnerability details

The current design/implementation allows a market address (registered on registry) to call Vault#addValue() and transfer tokens from an arbitrary address to a specified _beneficiary up the approved amount at any time, and the _beneficiary can withdraw the funds by calling Vault#withdrawAllAttribution() immediately.

This poses a very dangerous risk to all the users that approved their tokens to the Vault contracts (each one holds all users' allowances for that token).

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

modifier onlyMarket() {
    require(
        IRegistry(registry).isListed(msg.sender),
        "ERROR_ONLY_MARKET"
    );
    _;
}

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

function addValue(
    uint256 _amount,
    address _from,
    address _beneficiary
) external override onlyMarket returns (uint256 _attributions) {

    if (totalAttributions == 0) {
        _attributions = _amount;
    } else {
        uint256 _pool = valueAll();
        _attributions = (_amount * totalAttributions) / _pool;
    }
    IERC20(token).safeTransferFrom(_from, address(this), _amount);
    balance += _amount;
    totalAttributions += _attributions;
    attributions[_beneficiary] += _attributions;
}

Registry owner can call Registry#supportMarket() and mark an arbitrary address as a market.

https://github.com/code-423n4/2022-01-insure/blob/19d1a7819fe7ce795e6d4814e7ddf8b8e1323df3/contracts/Registry.sol#L49-L60

function supportMarket(address _market) external override {
    require(!markets[_market], "ERROR: ALREADY_REGISTERED");
    require(
        msg.sender == factory || msg.sender == ownership.owner(),
        "ERROR: UNAUTHORIZED_CALLER"
    );
    require(_market != address(0), "ERROR: ZERO_ADDRESS");

    allMarkets.push(_market);
    markets[_market] = true;
    emit NewMarketRegistered(_market);
}

Or, the owner of the Factory can call createMarket() to add a malicous market contract via a custom template contract to the markets list.

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

PoC

A malicious/compromised Registry owner can:

1. Call Registry#supportMarket() and set markets[attackerAddress] to true;
2. Call Vault#addValue(token.balanceOf(victimAddress), victimAddress, attackerAddress) and transferring all the balanceOf victim's wallet to the vault, owned by attackerAddress.
3. Call Vault#withdrawAllAttribution(attackerAddress) and retrive the funds.

The malicious/compromised Registry owner can repeat the steps above for all the users who approved the Vault contract for all the Vault contracts.

As a result, the attacker can steal all the wallet balances of the tokens approved to the protocol.

Root Cause

Improper access control for using users' allowances.

Recommendation

Consider changing the design/implementation to make sure that the allowances approved by the users can only be used by themselves.

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Handle

WatchPug

Vulnerability details

Root Cause

Precision loss while converting between the amount of shares and the amount of underlying tokens back and forth is not handled properly.

---

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

uint256 _shortage;
if (totalLiquidity() < _amount) {
    //Insolvency case
    _shortage = _amount - _value;
    uint256 _cds = ICDSTemplate(registry.getCDS(address(this)))
        .compensate(_shortage);
    _compensated = _value + _cds;
}
vault.offsetDebt(_compensated, msg.sender);

In the current implementation, when someone tries to resume the market after a pending period ends by calling PoolTemplate.sol#resume(), IndexTemplate.sol#compensate() will be called internally to make a payout. If the index pool is unable to cover the compensation, the CDS pool will then be used to cover the shortage.

However, while CDSTemplate.sol#compensate() takes a parameter for the amount of underlying tokens, it uses vault.transferValue() to transfer corresponding _attributions (shares) instead of underlying tokens.

Due to precision loss, the _attributions transferred in the terms of underlying tokens will most certainly be less than the shortage.

At L444, the contract believes that it's been compensated for _value + _cds, which is lower than the actual value, due to precision loss.

At L446, when it calls vault.offsetDebt(_compensated, msg.sender), the tx will revert at require(underlyingValue(msg.sender) >= _amount).

As a result, resume() can not be done, and the debt can't be repaid.

PoC

Given:

• vault.underlyingValue = 10,000
• vault.valueAll = 30,000
• totalAttributions = 2,000,000
• _amount = 1,010,000

0. _shortage = _amount - vault.underlyingValue = 1,000,000
1. _attributions = (_amount * totalAttributions) / valueAll = 67,333,333
2. actualValueTransfered = (valueAll * _attributions) / totalAttributions = 1009999

Expected results: actualValueTransfered = _shortage;

Actual results: actualValueTransfered < _shortage.

Impact

The precision loss isn't just happening on special numbers, but will most certainly always revert the txs.

This will malfunction the contract as the index pool can not compensate(), therefore the pool can not resume(). Causing the funds of the LPs of the pool and the index pool to be frozen, and other stakeholders of the same vault will suffer fund loss from an unfair share of the funds compensated before.

Recommendation

Change to:

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

if (totalLiquidity() < _amount) {
    //Insolvency case
    _shortage = _amount - _value;
    uint256 _cds = ICDSTemplate(registry.getCDS(address(this)))
        .compensate(_shortage);
    _compensated = vault.underlyingValue(address(this));
}
vault.offsetDebt(_compensated, msg.sender);

if (totalLiquidity() < _amount) {
    //Insolvency case
    _shortage = _amount - _value;
    uint256 _cds = ICDSTemplate(registry.getCDS(address(this)))
        .compensate(_shortage);
    if(_cds < _shortage) {
      _compensated = _value + _cds;
    } else {
      // To avoid reversion due to precision loss on "underlyingValue(msg.sender) >= _amount" in "offsetDebt"
      _compensated = vault.underlyingValue(msg.sender);
    }
}
vault.offsetDebt(_compensated, msg.sender);

cds.compensate(_amount)

vault.transferValue(_compensated, msg.sender)

_attributions = (_amount * totalAttributions) / _valueAll

vault.offsetDebt(_compensated, msg.sender);

require(underlyingValue(msg.sender, _valueAll) >= _amount)

return (_valueAll * attributions[_target]) / totalAttributions

value_1 >= attribution >= value_2

_compensated = vault.underlyingValue(index) 

Handle

WatchPug

Vulnerability details

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

function setController(address _controller) public override onlyOwner {
    require(_controller != address(0), "ERROR_ZERO_ADDRESS");

    if (address(controller) != address(0)) {
        controller.migrate(address(_controller));
        controller = IController(_controller);
    } else {
        controller = IController(_controller);
    }

    emit ControllerSet(_controller);
}

The owner of the Vault contract can set an arbitrary address as the controller.

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

function utilize() external override returns (uint256 _amount) {
    if (keeper != address(0)) {
        require(msg.sender == keeper, "ERROR_NOT_KEEPER");
    }
    _amount = available(); //balance
    if (_amount > 0) {
        IERC20(token).safeTransfer(address(controller), _amount);
        balance -= _amount;
        controller.earn(address(token), _amount);
    }
}

A malicious controller contract can transfer funds from the Vault to the attacker.

PoC

A malicious/compromised can:

1. Call Vault#setController() and set controller to a malicious contract;
   • L489 the old controller will transfer funds to the new, malicious controller.
2. Call Vault#utilize() to deposit all the balance in the Vault contract into the malicious controller contract.
3. Withdraw all the funds from the malicious controller contract.

Recommendation

Consider disallowing Vault#setController() to set a new address if a controller is existing, which terminates the possibility of migrating funds to a specified address provided by the owner. Or, putting a timelock to this function at least.

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Handle

WatchPug

Vulnerability details

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

modifier onlyMarket() {
    require(
        IRegistry(registry).isListed(msg.sender),
        "ERROR_ONLY_MARKET"
    );
    _;
}

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

The current design/implementation allows a market address (registered on the registry) to call Vault#borrowValue() and transfer tokens to an arbitrary address.

PoC

See the PoC section on [WP-H24].

Recommendation

1. Consider adding constrains (eg. timelock) to Registry#supportMarket().
2. Consdier adding constrains (upper bound for each pool, and index pool for example) to Vault#borrowValue().

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Handle

WatchPug

Vulnerability details

Root Cause

Wrong arithmetic.

---

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

uint256 _deductionFromIndex = (_debt * _totalCredit * MAGIC_SCALE_1E6) /
            totalLiquidity();
    uint256 _actualDeduction;
    for (uint256 i = 0; i < indexList.length; i++) {
        address _index = indexList[i];
        uint256 _credit = indicies[_index].credit;
        if (_credit > 0) {
            uint256 _shareOfIndex = (_credit * MAGIC_SCALE_1E6) /
                _totalCredit;
            uint256 _redeemAmount = _divCeil(
                _deductionFromIndex,
                _shareOfIndex
            );
            _actualDeduction += IIndexTemplate(_index).compensate(
                _redeemAmount
            );
        }
    }

PoC

• totalLiquidity = 200,000* 10**18;

• totalCredit = 100,000 * 10**18;

• debt = 10,000 * 10**18;

• [Index Pool 1] Credit = 20,000 * 10**18;

• [Index Pool 2] Credit = 30,000 * 10**18;

uint256 _deductionFromIndex = (_debt * _totalCredit * MAGIC_SCALE_1E6) /
            totalLiquidity();
// _deductionFromIndex = 10,000 * 10**6 * 10**18;

[Index Pool 1]:

uint256 _shareOfIndex = (_credit * MAGIC_SCALE_1E6) / _totalCredit;  
//  _shareOfIndex = 200000

uint256 _redeemAmount = _divCeil(
    _deductionFromIndex,
    _shareOfIndex
);

// _redeemAmount = 25,000 * 10**18;

[Index Pool 2]:

uint256 _shareOfIndex = (_credit * MAGIC_SCALE_1E6) / _totalCredit;  
//  _shareOfIndex = 300000

uint256 _redeemAmount = _divCeil(
    _deductionFromIndex,
    _shareOfIndex
);

// _redeemAmount = 16666666666666666666667 (~ 16,666 * 10**18)

In most cases, the transaction will revet on underflow at:

uint256 _shortage = _deductionFromIndex /
            MAGIC_SCALE_1E6 -
            _actualDeduction;

In some cases, specific pools will be liable for unfair compensation:

If the CSD is empty, Index Pool 1 only have 6,000 * 10**18 and Index Pool 2 only have 4,000 * 10**18, the _actualDeduction will be 10,000 * 10**18, _deductionFromPool will be 0.

Index Pool 1 should only pay 1,000 * 10**18, but actually paid 6,000 * 10**18, the LPs of Index Pool 1 now suffer funds loss.

Recommendation

Change to:

uint256 _deductionFromIndex = (_debt * _totalCredit * MAGIC_SCALE_1E6) / totalLiquidity();
uint256 _actualDeduction;
for (uint256 i = 0; i < indexList.length; i++) {
    address _index = indexList[i];
    uint256 _credit = indicies[_index].credit;
    if (_credit > 0) {
        uint256 _shareOfIndex = (_credit * MAGIC_SCALE_1E6) /
            _totalCredit;
        uint256 _redeemAmount = _divCeil(
            _deductionFromIndex * _shareOfIndex,
            MAGIC_SCALE_1E6 * MAGIC_SCALE_1E6
        );
        _actualDeduction += IIndexTemplate(_index).compensate(
            _redeemAmount
        );
    }
}

Handle

WatchPug

Vulnerability details

Based on the context, the system intends to lock all the lps during PayingOut period.

However, the current implementation allows anyone, including LPs to call resume() and unlock the index pool.

It allows a malicious LP to escape the responsibility for the compensation, at the expense of other LPs paying more than expected.

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

function resume() external override {
    uint256 _poolLength = poolList.length;

    for (uint256 i = 0; i < _poolLength; i++) {
        require(
            IPoolTemplate(poolList[i]).paused() == false,
            "ERROR: POOL_IS_PAUSED"
        );
    }

    locked = false;
    emit Resumed();
}

Recommendation

Change to:

function resume() external override {
   uint256 _poolLength = poolList.length;

   for (uint256 i = 0; i < _poolLength; i++) {
       require(
           IPoolTemplate(poolList[i]).marketStatus() == MarketStatus.Trading,
           "ERROR: POOL_IS_PAYINGOUT"
       );
   }

   locked = false;
   emit Resumed();
}

Handle

WatchPug

Vulnerability details

In the current implementation, when an incident is reported for a certain pool, the index pool can still withdrawCredit() from the pool, which in the best interest of an index pool, the admin of the index pool is preferred to do so.

This allows the index pool to escape from the responsibility for the risks of invested pools.

Making the LPs of the pool take an unfair share of the responsibility.

PoC

• Pool A totalCredit = 10,000
• Pool A rewardPerCredit = 1

1. [Index Pool 1] allocates 1,000 credits to Pool A:

• totalCredit = 11,000
• indicies[Index Pool 1] = 1,000

2. After a while, Pool A rewardPerCredit has grown to 1.1, and applyCover() has been called, [Index Pool 1] call withdrawCredit() get 100 premium

• totalCredit = 10,000
• indicies[Index Pool 1] = 0

3. After pendingEnd, the pool resume(),[ Index Pool 1] will not be paying for the compensation since credit is 0.

In our case, [Index Pool 1] earned premium without paying for a part of the compensation.

Recommendation

Change to:

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

    function withdrawCredit(uint256 _credit)
        external
        override
        returns (uint256 _pending)
    {
        require(
            marketStatus == MarketStatus.Trading,
            "ERROR: WITHDRAW_CREDIT_BAD_CONDITIONS"
        );
        IndexInfo storage _index = indicies[msg.sender];

Handle

WatchPug

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() / transferFrom().

Vault.sol#addValue() assumes that the received amount is the same as the transfer amount, and uses it to calculate attributions, balance amounts, etc. While the actual transferred amount can be lower for those tokens.

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

function addValue(
    uint256 _amount,
    address _from,
    address _beneficiary
) external override onlyMarket returns (uint256 _attributions) {

    if (totalAttributions == 0) {
        _attributions = _amount;
    } else {
        uint256 _pool = valueAll();
        _attributions = (_amount * totalAttributions) / _pool;
    }
    IERC20(token).safeTransferFrom(_from, address(this), _amount);
    balance += _amount;
    totalAttributions += _attributions;
    attributions[_beneficiary] += _attributions;
}

Recommendation

Consider comparing before and after balance to get the actual transferred amount.

Handle

WatchPug

Vulnerability details

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

function applyCover(
    uint256 _pending,
    uint256 _payoutNumerator,
    uint256 _payoutDenominator,
    uint256 _incidentTimestamp,
    bytes32 _merkleRoot,
    string calldata _rawdata,
    string calldata _memo
) external override onlyOwner {
    require(paused == false, "ERROR: UNABLE_TO_APPLY");
    incident.payoutNumerator = _payoutNumerator;
    incident.payoutDenominator = _payoutDenominator;
    incident.incidentTimestamp = _incidentTimestamp;
    incident.merkleRoot = _merkleRoot;
    marketStatus = MarketStatus.Payingout;
    pendingEnd = block.timestamp + _pending;
    for (uint256 i = 0; i < indexList.length; i++) {
        if (indicies[indexList[i]].credit > 0) {
            IIndexTemplate(indexList[i]).lock();
        }
    }
    emit CoverApplied(
        _pending,
        _payoutNumerator,
        _payoutDenominator,
        _incidentTimestamp,
        _merkleRoot,
        _rawdata,
        _memo
    );
    emit MarketStatusChanged(marketStatus);
}

An attacker can add a lot of IndexPool to the individual pool, if the length of indexList is large enough, the gas cost of function applyCover() can exceed the block limit, making it impossible to applyCover.

Essentially, this allows an attacker (as LP) to get the premium without taking the risk of payout.

Recommendation

Consider allowing applyCover for a certain range of IndexPool.