Sandclock contest - kenzo's results

Handle

kenzo

Vulnerability details

To calculate the total vault\strategy's assets, the strategy is using a spot price: curvePool.get_dy_underlying. This can be easily manipulatable by an exploiter (including using flash loans).

Impact

Wrong accounting:

• If exploiter temporarily decreases get_dy_underlying before depositing, he would get more shares than he deserves. He can then withdraw other user's funds. This can also prevent a real user from withdrawing his funds.
• If exploiter temporarily increases get_dy_underlying before claiming yield, he would get more yield than really exists. This might come on the expense of legitimate user's investment funds, and the user won't be able to fully withdraw until (and if) the vault makes enough real yield to cover the manipulation.

Proof of Concept

When I say that an exploiter can temporarily change the price, I mean that he can use flash loan funds (or his own funds) to execute a large trade on Curve before calling the vault (in the same tx). This will incur a slippage of price, that the vault will use as if it's the real price. After the vault action, the exploiter can swap back on Curve.

If exploiter temporarily decreases get_dy_underlying before depositing: Let's take this scenario:

• Alice deposits 100 USDT tokens
• Alice's funds are converted to UST and deposited into EthAnchor
• Bob manipulates Curve's get_dy_underlying such that the value of the 100 UST tokens is now 90 UST tokens. He then deposits 100 USDT.

At this point, when depositing Bob's funds, the shares mechanism would call get_dy_underlying to calculate the total underlying, and will think that the vault is in a loss. It would give Bob shares, and mint global shares, as if the vault's underlying is 90, although it is actually 100.

Now let's say Alice tries to withdraw - The shares mechanism would look at the vault's balance and will see that it has increased by 10 since Bob deposited. Since (at Bob's deposit) Sandclock minted global shares as if Alice's underlying is only 90, of these "new" 10 tokens that Sandclock now sees, only (90 / 190)*10 = 4.7 belong to Alice. So Sandclock thinks that Alice is in a loss and can not withdraw her funds yet, although that is false. So Alice can not withdraw her funds.

Now let's say Bob claims yield - As mentioned before, the shares mechanism will calculate that of the 10 "new" underlying, 100/190 belong to Bob. Bob seems to be in profit, so he will be able to claim "his share" of the "yield" - which is actually partly Alice's investment.

[Note: this scenario can be easily verified by adding a function to the vault that will transfer underlying out before Bob deposits. This can be considered to simulate a decreasing get_dy_underlying and therefore smaller totalUnderlying. Then after Bob deposited, you can mint underlying back to the vault to simulate the price getting back to normal.]

If exploiter temporarily increases get_dy_underlying before claiming yield: In this scenario, both Alice and Bob the exploiter (not Bob the builder) have deposited as normal. Bob then manipulatively increases get_dy_underlying before claiming yield. The shares mechanism will think there is yield to claim, which it will send to Bob. But this yield is actually Alice and Bob's investment. This process can then be repeated. So again, Bob is getting funds which are not his. Damn it, Bob.

[Note: this scenario can be verified by adding a global variable uint256 manipulation, and adding it to totalUnderlying()'s calculation. Then set it to some amount like 10 ether before Bob claims his yield, and set it back to 0 after. This will simulate manipulation of the strategy's invested assets.]

Recommended Mitigation Steps

Don't use a plain spot price as price feed. Use some kind of Oracle.

Handle

kenzo

Vulnerability details

Upon withdrawal of deposit, the claimer will be called with onDepositBurned. This happens after the claimer shares have been updated, but before the underlying has been sent away from the contract. Therefore the claimer can reenter the contract, at an intermediary state where the invariants are not held.

Impact

Claimer can claim more yield than he deserves.

Proof of Concept

The withdraw function iterates on all deposits, and then sends underlying only at the end. For each deposit (while iterating), Sandclock first updates the shares state, and then calls onDepositBurned. So the claimer can reenter the vault after the shares have been updated but before underlying was sent.

POC for exploit:

• Alice deposits 100 tokens, with claims that go 50% to bob and 50% to exploiter
• Vault generates 10 yield
• Alice withdraws the deposit which has exploiter as claimed
• The exploiter, on onDepositBurned, claims his yield from the contract. Since the underlying has not been sent yet, the state is not consistent. Exploiter gets 9.16 yield instead of 5.

Recommended Mitigation Steps

Usual reentrancy protection: conform with checks-effects-interaction pattern, or add reentrancy lock.

Handle

kenzo

Vulnerability details

EthAnchor's docs state that "the contract address of ExchangeRateFeeder may change as adjustments occur". BaseStrategy does not have a setter to change exchangeRateFeeder after deployment.

Impact

Inaccurate/unupdated values from exchangeRateFeeder when calculating vault's total invested assets.

While the strategy's funds could be withdrawn from EthAnchor and migrated to a new strategy with correct exchangeRateFeeder, during this process (which might take time due to EthAnchor's async model) the wrong exchangeRateFeeder will be used to calculate the vault's total invested assets. (The vault's various actions (deposit, claim, withdraw) can not be paused.)

Proof of Concept

The exchangeRateFeeder is being used to calculate the vault's invested assets, which is used extensively to calculate the correct amount of shares and amounts: (Code ref)

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

EthAnchor documentation states that unlike other contracts, exchangeRateFeeder is not proxied and it's address may change in future: "the contract address of ExchangeRateFeeder may change as adjustments occur. " (ref)

Recommended Mitigation Steps

Add a setter for exchangeRateFeeder.

Handle

kenzo

Vulnerability details

A vault wouldn't let the strategy be changed unless the strategy holds no funds. Since anybody can send funds to the strategy, a griefing attack is possible.

Impact

Strategy couldn't be changed.

Proof of Concept

setStrategy requires strategy.investedAssets() == 0. (Code ref) investedAssets contains the aUST balance and the pending redeems: (Code ref)

uint256 aUstBalance = _getAUstBalance() + pendingRedeems;

So if a griefer sends 1 wei of aUST to the strategy before it is to be replaced, it would not be able to be replaced. The protocol would then need to redeem the aUST and wait for the process to finish - and the griefer can repeat his griefing. As they say, griefers gonna grief.

Recommended Mitigation Steps

Consider keeping an internal aUST balance of the strategy, which will be updated upon deposit and redeem, and use it (instead of raw aUST balance) to check if the strategy holds no aUST funds. Another option is to add capability for the strategy to send the aUST to the vault.