Sandclock contest - defsec's results

Handle

defsec

Vulnerability details

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.

Proof of Concept

The NonUSTStrategy contract is 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 curvePool.exchange_underlying function : https://github.com/code-423n4/2022-01-sandclock/blob/main/sandclock/contracts/strategy/NonUSTStrategy.sol

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

• According curve pool documentation the following parameter shows the minimum return amount of token on the return. On the contract, It is defined as 0.

https://curve.readthedocs.io/exchange-pools.html
_min_dy: Minimum amount of j to receive

Tools Used

Code Review

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 such that they are not visible in the public mempool.

Handle

defsec

Vulnerability details

Impact

The re-entrancy guard is missing on the Eth anchor interaction. The external router interaction can cause to the re-entrancy vulnerability.

Proof of Concept

1. Navigate to the following contract.

    function finishDepositStable(uint256 idx) external {
        require(depositOperations.length > idx, "not running");
        Operation storage operation = depositOperations[idx];
        ethAnchorRouter.finishDepositStable(operation.operator);

        pendingDeposits -= operation.amount;
        convertedUst += operation.amount;

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

Tools Used

Code Review

Recommended Mitigation Steps

Follow the check effect interaction pattern or put re-entrancy guard.

Handle

defsec

Vulnerability details

Impact

The Strategy contracts do not appear to support rebasing/deflationary/inflationary tokens whose balance changes during transfers or over time. The necessary checks include at least verifying the amount of tokens transferred to contracts before and after the actual transfer to infer any fees/interest.

Proof of Concept

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

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

Tools Used

Code Review

Recommended Mitigation Steps

Make sure token vault accounts for any rebasing/inflation/deflation Add support in contracts for such tokens before accepting user-supplied tokens Consider to check before/after balance on the vault.