InsureDAO contest - cmichel's results

Handle

cmichel

Vulnerability details

Note that the PoolTemplate.initialize function, called when creating a market with Factory.createMarket, calls a vault function to transfer an initial deposit amount (conditions[1]) from the initial depositor (_references[4]):

// PoolTemplate
function initialize(
     string calldata _metaData,
     uint256[] calldata _conditions,
     address[] calldata _references
) external override {
     // ...

     if (_conditions[1] > 0) {
          // @audit vault calls asset.transferFrom(_references[4], vault, _conditions[1])
          _depositFrom(_conditions[1], _references[4]);
     }
}

function _depositFrom(uint256 _amount, address _from)
     internal
     returns (uint256 _mintAmount)
{
     require(
          marketStatus == MarketStatus.Trading && paused == false,
          "ERROR: DEPOSIT_DISABLED"
     );
     require(_amount > 0, "ERROR: DEPOSIT_ZERO");

     _mintAmount = worth(_amount);
     // @audit vault calls asset.transferFrom(_from, vault, _amount)
     vault.addValue(_amount, _from, address(this));

     emit Deposit(_from, _amount, _mintAmount);

     //mint iToken
     _mint(_from, _mintAmount);
}

The initial depositor needs to first approve the vault contract for the transferFrom to succeed.

An attacker can then frontrun the Factory.createMarket transaction with their own market creation (it does not have access restrictions) and create a market with different parameters but still passing in _conditions[1]=amount and _references[4]=victim.

A market with parameters that the initial depositor did not want (different underlying, old whitelisted registry/parameter contract, etc.) can be created with their tokens and these tokens are essentially lost.

Recommended Mitigation Steps

Can the initial depositor be set to Factory.createMarket's msg.sender, instead of being able to pick a whitelisted one as _references[4]?

Handle

cmichel

Vulnerability details

The Vault.withdrawRedundant has wrong logic that allows the admins to steal the underlying vault token.

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) {
          // @audit they can rug users. let's say balance == IERC20(token).balanceOf(address(this)) => first if false => transfers out everything
          IERC20(_token).safeTransfer(
               _to,
               IERC20(_token).balanceOf(address(this))
          );
     }
}

POC

• Vault deposits increase as Vault.addValue is called and the balance increases by _amount as well as the actual IERC20(token).balanceOf(this). Note that balance == IERC20(token).balanceOf(this)
• Admins call vault.withdrawRedundant(vault.token(), attacker) which goes into the else if branch due to the balance inequality condition being false. It will transfer out all vault.token() amounts to the attacker.

Impact

There's a backdoor in the withdrawRedundant that allows admins to steal all user deposits.

Recommended Mitigation Steps

I think the devs wanted this logic from the code instead:

function withdrawRedundant(address _token, address _to)
     external
     override
     onlyOwner
{
     if (
          _token == address(token)
     ) {
          if (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))
          );
     }
}

Handle

cmichel

Vulnerability details

Certain ERC20 tokens make modifications to their ERC20's transfer or balanceOf functions. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

Impact

The Vault.addValue(Batch) functions will recive less tokens than specified because they receive the amount - fees:

function addValueBatch(
     uint256 _amount,
     address _from,
     address[2] memory _beneficiaries,
     uint256[2] memory _shares
) external override onlyMarket returns (uint256[2] memory _allocations) {
     // ...
     IERC20(token).safeTransferFrom(_from, address(this), _amount);

     // @audit does not work with fee-on-transfer
     balance += _amount;
     // ...
}

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring calls.