Sandclock contest - cmichel's results

Handle

cmichel

Vulnerability details

The Vault._withdraw function first caches the current total shares and underlying, and then iterates over all deposit NFT _ids using a call to _withdrawDeposit. Only afterwards, does it pay out the accumulated withdrawn amount.

function _withdraw(
    address _to,
    uint256[] memory _ids,
    bool _force
) internal {
    // @audit caches values here and uses cached values for all `_withdrawDeposit` loop iterations
    uint256 localTotalShares = totalShares();
    uint256 localTotalPrincipal = totalUnderlyingMinusSponsored();
    uint256 amount;

    for (uint8 i = 0; i < _ids.length; i++) {
        // @audit totalShares() changes here whenever claim.onWithdraw is called
        // @audit can re-enter here
        amount += _withdrawDeposit(
            _ids[i],
            localTotalShares,
            localTotalPrincipal,
            _to,
            _force
        );
    }
    // @audit totalUnderlyingMinusSponsored() changes here
    underlying.safeTransfer(_to, amount);
}

The _withdrawDeposit function gives control to the attacker in the IIntegration(claimer).onDepositBurned(_tokenId); call. Notice how the totalShares() value changes in each loop iteration as tokens are burned in the claim.onWithdraw call, but the totalUnderlyingMinusSponsored() only changes once at the end of the function. Meaning, control is given to the attacker while total shares have been decreased but the underlying amount has not yet decreased. This allows stealing tokens as the _computeShares/_computeAmount math only works correctly when both values are decreased atomically.

POC

For simplicity, assume shares and amounts are 1-to-1, therefore totalShares = 8k, the totalUnderlyingMinusSponsored = 8k.

1. The attacker deposits 1k underlying twice to receive two deposit and two claim NFTs with id=1, id=2. They set the _claim1.beneficiary = attackerContract1, _claim2.beneficiary = attackerContract2 and use the minimum lock period. (They send NFT id=2 to attackerContract1 such that it is the owner and can re-enter later)
2. The new totalShares = 10k, the totalUnderlyingMinusSponsored = 10k.
3. They wait until the lock period has passed so they can withdraw
4. They call withdraw(_to=attacker, _ids=[1]) which calls _withdraw(_to, ids, false). Then _withdrawDeposit(_id=1, localTotalShares=10k, localTotalPrincipal=10k, _to=attacker, _force=false) is called. The following values are computed depositAmount = 1k, depositShares = 10k * 1k / 10k = 1k. The claimers.onWithdraw(..., 1k) call reduces the totalShares() to 9k. The attacker receives control through the IIntegration(claimer=attackerContract1).onDepositBurned(_tokenId) call.
5. The attacker contract calls withdraw(_to=attacker, _ids=[2]) caches localTotalShares = 9k, localTotalPrincipal=10k. The total shares has been reduced while the underlying amount has not as it has not been paid out yet. The _withdrawDeposit computes depositAmount = 1k, depositShares = 9k * 1k / 10k = 900 and a return value of _computeAmount(depositShares=900, _totalShares=9k, _totalUnderlyingMinusSponsored=10k) = 1k. The issue here is that only 900 shares of the 1000 shares in the claim NFT are burned through the claim.onWithdraw call. The claim NFT still has 100 shares and a principal of 0. The total shares is reduced to 9000 - 900 = 8100.
6. All calls finish and the attacker receives their initial 1k deposit amount twice.
7. The non-empty shares in the claim NFT are their profit which they can now get through claimYield(_to=attacker): They receive a yield of currentClaimerPrincipal - claimerPrincipal = 8000 * 100 / 8100 - 0 = 98 free tokens

Impact

The parameters (number of NFTs, recursion depth, deposit amounts) can be optimized to steal all tokens.

Recommended Mitigation Steps

Add re-entrancy guards to all user-facing, public functions of the vault.

Handle

cmichel

Vulnerability details

The contracts are missing slippage checks which can lead to being vulnerable to sandwich attacks.

A common attack in DeFi is the sandwich attack. Upon observing a trade of asset X for asset Y, an attacker frontruns the victim trade by also buying asset Y, lets the victim execute the trade, and then backruns (executes after) the victim by trading back the amount gained in the first trade. Intuitively, one uses the knowledge that someone’s going to buy an asset, and that this trade will increase its price, to make a profit. The attacker’s plan is to buy this asset cheap, let the victim buy at an increased price, and then sell the received amount again at a higher price afterwards.

See NonUSTStrategy._swapUnderlyingToUst/_swapUstToUnderlying:

function _swapUnderlyingToUst() internal {
    uint256 underlyingBalance = _getUnderlyingBalance();
    if (underlyingBalance > 0) {
        // slither-disable-next-line unused-return
        curvePool.exchange_underlying(
            underlyingI,
            ustI,
            underlyingBalance,
            // @audit minReturn of 0 => can sandwich
            0
        );
    }
}

/**
  * Calls Curve to convert the existing UST back into the underlying token
  */
function _swapUstToUnderlying() internal {
    uint256 ustBalance = _getUstBalance();
    if (ustBalance > 0) {
        // slither-disable-next-line unused-return
        // @audit minReturn of 0 => can sandwich
        curvePool.exchange_underlying(ustI, underlyingI, ustBalance, 0);
    }
}

Impact

Trades can happen at a bad price and lead to receiving fewer tokens than at a fair market price. The attacker's profit is the protocol's loss.

Recommended Mitigation Steps

Add minimum return amount checks. Accept a function parameter that can be chosen by the transaction sender, then check that the actually received amount is above this parameter.

Alternatively, check if it's feasible to send these transactions directly to a miner (flashbots) such that they are not visible in the public mempool.

Handle

cmichel

Vulnerability details

The Vault._createDeposit function first caches the current total shares and underlying, and then iterates over all claims using a call to _createClaim. Only afterwards, does it pull in the required total amount in the deposit.

function deposit(DepositParams calldata _params) external {
    _createDeposit(_params.amount, _params.lockedUntil, _params.claims);
    _transferAndCheckUnderlying(_msgSender(), _params.amount);
}

function _createDeposit(
        uint256 _amount,
    uint256 _lockedUntil,
    ClaimParams[] calldata claims
) internal {
    if (_lockedUntil == 0) _lockedUntil = block.timestamp + minLockPeriod;
    else
        require(
            _lockedUntil >= block.timestamp + minLockPeriod,
            "Vault: lock time is too small"
        );
    /* @audit uses cached values, combined with re-entrancy issue?
        underlying increases at the end at caller of _createDeposit
        shares increase each iteration
        1. deposit 1k => mint 1k shares, totalShares() = 11k, => re-enter through deposits.mint _safeMint
        2. deposit 1k => mint 1k/10k * 11k = 1100 shares
    */
    uint256 localTotalShares = totalShares();
    uint256 localTotalUnderlying = totalUnderlyingMinusSponsored();
    uint256 pct = 0;

    for (uint256 i = 0; i < claims.length; ++i) {
        ClaimParams memory data = claims[i];
        _createClaim(
            _depositGroupIds.current(),
            _amount,
            _lockedUntil,
            data,
            localTotalShares,
            localTotalUnderlying
        );
        pct += data.pct;
    }

    _depositGroupIds.increment();

    require(pct.is100Perc(), "Vault: claims don't add up to 100%");
}

The _createClaim function gives control to the attacker in the depositors.mint(_msgSender(), ...) call. (Or through the beneficiary onDepositMinted callback.) Notice how the totalShares() value changes in each loop iteration as tokens are minted in the claimers.mint call, but the totalUnderlyingMinusSponsored() only changes once at the end of the deposit function by pulling in the deposited amount. Meaning, control is given to the attacker while total shares has been increased but the underlying amount has not yet increased. This allows stealing tokens as the _computeShares/_computeAmount math only works correctly when both values are increased atomically.

POC

For simplicity, assume shares and amounts are 1-to-1, therefore totalShares = 10k, the totalUnderlyingMinusSponsored = 10k. Note that depositing 2k underlying should give someone 2k shares under normal circumstances. The attacker is able to deposit 2k underlying but receive more shares for it.

1. The attacker contract calls deposit(amount=1k, lockedUntil=min, claims=[100%]). Then _createClaim(groupId, _amount=1k, _claim, _localTotalShares=10k, _localTotalPrincipal=10k is called which computes newShares = 1k * 10k / 10k = 1k
2. The attacker receives 1k shares in the claim NFT. The new totalShares() = 11k. The depositors.mint(_msgSender(), ...) call leads to a _safeMint(_owner, localTokenId) call which calls attackerContract.onERC721Received and gives control to the attacker.
3. They deposit another 1k underlying by calling deposit(amount=1k, lockedUntil=min, claims=[100%]). This time the newShares = 1k * 11k / 10k = 1100 as the totalShares() has been increased but the underlying amount has not. The new total shares is 11000 + 1100 = 12100
4. All in all, the attacker received 2100 shares for paying 2000 underlying.
5. They wait until the min unlock period is over
6. They withdraw their 2100 shares and receive 2100 / 12100 * 12000 = 2082. They made a profit of 82 underlying, stealing from other depositors

Impact

The parameters (number of NFTs, recursion depth, deposit amounts) can be optimized to steal all tokens.

Recommended Mitigation Steps

Add re-entrancy guards to all user-facing, public functions of the vault.

Handle

cmichel

Vulnerability details

When depositors want their funds back and there are not enough funds in the vault or the strategy, the aUST in the BaseStrategy needs to be redeemed. This redemption process is asynchronous due to the nature of EthAnchor and requires an admin to redeem the aUST back to UST. It can only be started by the restricted initRedeemStable function.

Impact

If the admins don't redeem the aUST, users cannot get back their investment and lose funds.

Recommended Mitigation Steps

The users should not have to rely on admins to get their funds back.