InsureDAO contest - Ruhum's results

Handle

Ruhum

Vulnerability details

Impact

The Vault.withdrawRedundant() allows the owner to withdraw funds that are not accounted for. The function has a check that is supposed to stop the owner from withdrawing funds of the vault's underlying token that the vault "knows" about.

But, there's an edge case where the owner is able to take out the vault's whole balance of the underlying token.

Since the owner is able to simply send the tokens back, it's not that big of a deal. But, I think that's behavior that is unintended so I still wanna raise the issue here.

Proof of Concept

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

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

If _token == address(token) and balance == IERC20(token).balanceOf(address(this) the first if statement is false. Because it checks whether the balance is <. So if balance == IERC20(token).balanceOf(address(this), which should be default state of the vault, the else if block is executed. There, the whole balance is sent to _to.

Tools Used

none

Recommended Mitigation Steps

Add another if clause like this:

 if (
            _token == address(token) &&
            balance == IERC20(token).balanceOf(address(this))
        ) {
  // do nothing
}

Handle

Ruhum

Vulnerability details

Impact

Some tokens charge a fee for each transfer. USDT, for example, has the possibility of enabling fees at any time. If the vault is used for that kind of token, the internal balance keeping will be wrong. The vault will think that it owns more tokens than it actually does.

The vault would allow users to withdraw, in total, more funds than it actually owns. Meaning at some point someone will lose tokens as there is not enough to cover their share.

Proof of Concept

state var: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L35

Vault doesn't check the number of tokens it actually received from the transfer: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L136-L137

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

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

It's always an issue when the vault receives funds. If it sends them somewhere else the bookkeeping is correct.

Tools Used

none

Recommended Mitigation Steps

One possibility would be to ditch the internal bookkeeping and simply use the balance from the token contract.

Handle

Ruhum

Vulnerability details

Impact

Because of the block gas limit, looping over a dynamic array that grows over time might result in a DoS at some point.

Both the PoolTemplate and the IndexTemplate have such dynamic arrays. Both don't have any functionality to decrease the size. Meaning, if the array grows too big at some point there's no way to fix the issue. Parts of the protocol simply become unusable. Each function that loops over the array might at some point use more gas than is possible.

The size of both arrays is controlled by the owner of the contracts. Meaning, the user can't force a DoS here. But, there's nothing stopping the owner from increasing the size to a point where a DoS happens.

Here's the SWC entry: https://swcregistry.io/docs/SWC-128

Proof of Concept

IndexPool:

• dynamic array: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L74
• loops:
  • https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L280
  • https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L348
  • https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L462
  • https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L655

PoolTemplate:

• dynamic array: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L93
• loops:
  • https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L671
  • https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L703

Tools Used

none

Recommended Mitigation Steps

Implement logic that allows breaking up the loop into multiple transactions. Or, avoid looping over a dynamic array at all.