Sandclock contest - leastwood's results

Handle

leastwood

Vulnerability details

Impact

The strategy contract intends to take invested assets allocated by the strategy's vault and bridge these assets to the Anchor protocol to accrue interest. Deposit and redeem actions are broken into init and finish components. The init component must be initiated by a subset of restricted accounts.

NonUSTStrategy.sol allows the vault's underlying token to be set to something other than UST by converting the underlying token to and from UST depending on if the assets are being redeemed or deposited. For example, upon depositing invested assets, _swapUnderlyingToUst() is used to convert the underlying tokens to UST before bridging. _swapUstToUnderlying() performs the opposite swap action when redeeming bridged tokens.

Curve's exchange_underlying() function is utilised in both of the aforementioned swap functions. The last parameter enforces a minimum number of output tokens, however, in the case of NonUSTStrategy.sol, this value is always set to 0. Therefore, any user can sandwich attack token swaps by using a flash loan and calling finishDepositStable() or finishRedeemStable(). This forces the strategy to swap tokens at an unfair and manipulated price, extracting value from the protocol.

Proof of Concept

https://github.com/curvefi/curve-contract/blob/master/integrations.md#exchanging-underlying-tokens

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/NonUSTStrategy.sol#L74-L85

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

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/NonUSTStrategy.sol#L90-L96

    function _swapUstToUnderlying() internal {
        uint256 ustBalance = _getUstBalance();
        if (ustBalance > 0) {
            // slither-disable-next-line unused-return
            curvePool.exchange_underlying(ustI, underlyingI, ustBalance, 0);
        }
    }

Tools Used

Manual code review. Discussions with Ryuhei.

Recommended Mitigation Steps

Consider adding necessary slippage checks when swapping between the underlying token and UST. This should be parsed as the min_dy parameter in exchange_underlying(). The affected functions include _swapUnderlyingToUst() and _swapUstToUnderlying().

Handle

leastwood

Vulnerability details

Impact

The vault contract aims to act as the interface by which users deposit and withdraw their assets. Upon depositing, users receive shares representing their claim on the vault's assets and any yield generated. Their position is minted as an NFT which is later burnt upon redeeming their deposited assets.

The _createDeposit() function queries totalShares() and totalUnderlyingMinusSponsored() to later calculate the amount of shares to mint in _createClaim(). There are two vulnerable external calls made which allow the user to reenter the deposit() function, namely:

• depositors.mint() which uses _safeMint() (containing a _checkOnERC721Received() call to _msgSender()).
• IIntegration(_claim.beneficiary).onDepositMinted() which is called if _isIntegration() is satisfied on _claim.beneficiary.

Each time the attacker reenters the deposit() function, the value for totalShares() is updated to reflect the newly minted shares, however, totalUnderlyingMinusSponsored() utilises an outdated amount as the _transferAndCheckUnderlying() is yet to be executed. As a result, _createClaim() will actually mint additional shares for the attacker if they break their deposit up into smaller amounts and reenter the function as often as possible. The attacker is able to enter the protocol at an improved exchange rate when compared to other users. This can be repeated again and again to continuously extract value from the protocol.

This issue also affects how shares are burnt when withdrawing deposits from the vault.

Proof of Concept

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L160-L163

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

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L420-L452

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

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

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L454-L505

function _createClaim(
    uint256 _depositGroupId,
    uint256 _amount,
    uint256 _lockedUntil,
    ClaimParams memory _claim,
    uint256 _localTotalShares,
    uint256 _localTotalPrincipal
) internal {
    uint256 amount = _amount.percOf(_claim.pct);

    uint256 newShares = _computeShares(
        amount,
        _localTotalShares,
        _localTotalPrincipal
    );

    uint256 claimerId = claimers.mint(
        _claim.beneficiary,
        amount,
        newShares
    );

    uint256 tokenId = depositors.mint(
        _msgSender(),
        amount,
        claimerId,
        _lockedUntil
    );

    if (_isIntegration(_claim.beneficiary)) {
        bytes4 ret = IIntegration(_claim.beneficiary).onDepositMinted(
            tokenId,
            newShares,
            _claim.data
        );

        require(
            ret == IIntegration(_claim.beneficiary).onDepositMinted.selector
        );
    }

    emit DepositMinted(
        tokenId,
        _depositGroupId,
        amount,
        newShares,
        _msgSender(),
        _claim.beneficiary,
        claimerId,
        _lockedUntil
    );
}

Tools Used

Manual code review. Discussions with the Sandclock team.

Recommended Mitigation Steps

Consider adding reentrancy protections to deposit(), withdraw() and all other sensitive functions in Vault.sol. Care needs to be taken whenever the checks-effects pattern is not implemented to ensure state variables such as totalShares() and totalUnderlyingMinusSponsored() cannot be manipulated by an attacker.

Handle

leastwood

Vulnerability details

Impact

Any user is allowed to sponsor a vault by depositing tokens, thereby bootstrapping the vault's initial assets used in the strategy contract. A percentage of these assets are invested in the Anchor protocol by bridging UST from Ethereum to the Terra blockchain.

As other participants continue to deposit funds, it is expected that the sponsored amount will be diluted over time. However, a well-funded user could inhibit user experience by sponsoring a large token amount, then wait for a trusted account to call Vault.updateInvested() and subsequently withdraw their deposited amount, reducing the vault's underlying token balance. As a result, users will likely be unable to withdraw their assets until other users enter the vault.

This can be performed as a form of denial of service as users are potentially unable to withdraw due to insufficient token balance in the vault contract. This issue is of medium severity as the issue impacts protocol availability.

Proof of Concept

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L213-L223

function investableAmount() public view returns (uint256) {
    uint256 maxInvestableAssets = totalUnderlying().percOf(investPerc);

    uint256 alreadyInvested = strategy.investedAssets();

    if (alreadyInvested >= maxInvestableAssets) {
        return 0;
    } else {
        return maxInvestableAssets - alreadyInvested;
    }
}

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L226-L238

function updateInvested() external requiresTrust {
    require(address(strategy) != address(0), "Vault: strategy is not set");

    uint256 _investable = investableAmount();

    if (_investable > 0) {
        underlying.safeTransfer(address(strategy), _investable);

        emit Invested(_investable);
    }

    strategy.doHardWork();
}

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/Vault.sol#L244-L267

function sponsor(uint256 _amount, uint256 _lockedUntil)
    external
    override(IVaultSponsoring)
{
    if (_lockedUntil == 0)
        _lockedUntil = block.timestamp + MIN_SPONSOR_LOCK_DURATION;
    else
        require(
            _lockedUntil >= block.timestamp + MIN_SPONSOR_LOCK_DURATION,
            "Vault: lock time is too small"
        );

    uint256 tokenId = depositors.mint(
        _msgSender(),
        _amount,
        0,
        _lockedUntil
    );

    emit Sponsored(tokenId, _amount, _msgSender(), _lockedUntil);

    totalSponsored += _amount;
    _transferAndCheckUnderlying(_msgSender(), _amount);
}

Tools Used

Manual code review.

Recommended Mitigation Steps

A clean solution would involve withdrawing the amount needed for a vault withdrawal whenever the vault has insufficient funds at hand. However, withdrawing from Anchor protocol takes some time, so this is not a feasible solution.

Therefore, it may be worthwhile to consider implementing a buffer which prevents Vault.updateInvested() from utilising all of the vault's assets, keeping some amount for withdrawals.

Handle

leastwood

Vulnerability details

Impact

The investedAssets() function is implemented by the vault's strategy contracts as a way to express a vault's investments in terms of the underlying currency. While the implementation of this function in BaseStrategy.sol and NonUSTStrategy.sol is mostly correct. It does not account for the performance fee charged by the treasury as shown in finishRedeemStable().

Therefore, an attacker could avoid paying their fair share of the performance fee by withdrawing their assets before several calls to finishRedeemStable() are made and reenter the vault once the fee is charged.

Proof of Concept

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/BaseStrategy.sol#L180-L204

function finishRedeemStable(uint256 idx) public virtual {
    require(redeemOperations.length > idx, "not running");
    Operation storage operation = redeemOperations[idx];
    uint256 aUstBalance = _getAUstBalance() + pendingRedeems;
    uint256 originalUst = (convertedUst * operation.amount) / aUstBalance;
    uint256 ustBalanceBefore = _getUstBalance();

    ethAnchorRouter.finishRedeemStable(operation.operator);

    uint256 redeemedAmount = _getUstBalance() - ustBalanceBefore;
    uint256 perfFee = redeemedAmount > originalUst
        ? (redeemedAmount - originalUst).percOf(perfFeePct)
        : 0;
    if (perfFee > 0) {
        ustToken.safeTransfer(treasury, perfFee);
        emit PerfFeeClaimed(perfFee);
    }
    convertedUst -= originalUst;
    pendingRedeems -= operation.amount;

    operation.operator = redeemOperations[redeemOperations.length - 1]
        .operator;
    operation.amount = redeemOperations[redeemOperations.length - 1].amount;
    redeemOperations.pop();
}

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/BaseStrategy.sol#L263-L277

function investedAssets()
    external
    view
    virtual
    override(IStrategy)
    returns (uint256)
{
    uint256 underlyingBalance = _getUnderlyingBalance() + pendingDeposits;
    uint256 aUstBalance = _getAUstBalance() + pendingRedeems;

    return
        underlyingBalance +
        ((exchangeRateFeeder.exchangeRateOf(address(aUstToken), true) *
            aUstBalance) / 1e18);
}

https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/NonUSTStrategy.sol#L120-L136

function investedAssets()
    external
    view
    override(BaseStrategy)
    returns (uint256)
{
    uint256 underlyingBalance = _getUnderlyingBalance();
    uint256 aUstBalance = _getAUstBalance() + pendingRedeems;

    uint256 ustAssets = ((exchangeRateFeeder.exchangeRateOf(
        address(aUstToken),
        true
    ) * aUstBalance) / 1e18) + pendingDeposits;
    return
        underlyingBalance +
        curvePool.get_dy_underlying(ustI, underlyingI, ustAssets);
}

Tools Used

Manual code review. Discussions with the Sandclock team (mostly Ryuhei).

Recommended Mitigation Steps

When calculating the investedAssets() amount (expressed in the underlying currency), consider calculating the expected performance fee to be charged if all the strategy's assets are withdrawn from the Anchor protocol. This should ensure that investedAssets() returns the most accurate amount, preventing users from gaming the protocol.

Handle

leastwood

Vulnerability details

Impact

The requiresTrust() modifier is used on the strategy, vault and factory contracts to prevent unauthorised accounts from calling restricted functions. Once an account is considered trusted, they are allowed to add and remove accounts by calling setIsTrusted() as they see fit.

However, if any single account has its private keys compromised or decides to become malicious on their own, they can remove all other trusted accounts from the isTrusted mapping. As a result, they are effectively able to take over the trusted group that controls all restricted functions in the parent contract.

Proof of Concept

abstract contract Trust {
    event UserTrustUpdated(address indexed user, bool trusted);

    mapping(address => bool) public isTrusted;

    constructor(address initialUser) {
        isTrusted[initialUser] = true;

        emit UserTrustUpdated(initialUser, true);
    }

    function setIsTrusted(address user, bool trusted) public virtual requiresTrust {
        isTrusted[user] = trusted;

        emit UserTrustUpdated(user, trusted);
    }

    modifier requiresTrust() {
        require(isTrusted[msg.sender], "UNTRUSTED");

        _;
    }
}

Tools Used

Manual code review.

Recommended Mitigation Steps

Consider utilising Rari Capital's updated Auth.sol contract found here. This updated contract gives the owner account authority over its underlying trusted accounts, preventing any single account from taking over the trusted group. The owner account should point to a multisig managed by the Sandclock team or by a community DAO.