yAxis contest - jonah1005's results

Handle

jonah1005

Vulnerability details

Impact

In controller.sol's function setCap, the contract wrongly handles _vaultDetails[_vault].balance. While the balance should be decreased by the difference of strategies balance, it subtracts the remaining balance of the strategy. Controller.sol#L262-L278 _vaultDetails[_vault].balance = _vaultDetails[_vault].balance.sub(_balance);

This would result in vaultDetails[_vault].balance being far smaller than the strategy's value. A user would trigger the assertion at Contreller.sol#475 and the fund would be locked in the strategy.

Though setCap is a permission function that only the operator can call, it's likely to be called and the fund would be locked in the contract. I consider this a high severity issue.

Proof of Concept

We can trigger the issue by setting the cap 1 wei smaller than the strategy's balance.

strategy_balance = strategy.functions.balanceOf().call()
controller.functions.setCap(vault.address, strategy.address, strategy_balance - 1, dai.address).transact()

## this would be reverted
vault.functions.withdrawAll(dai.address).transact()

Controller.sol#L262-L278

Tools Used

Hardhat

Recommended Mitigation Steps

I believe the dev would spot the issue in the test if _vaultDetails[_vault].balance is a public variable.

One possile fix is to subtract the difference of the balance.

uint previousBalance = IStrategy(_strategy).balanceOf();
_vaultDetails[_vault].balance.sub(previousBalance.sub(_amount));

Handle

jonah1005

Vulnerability details

Impact

There's no safety check in Manager.sol addToken. There are two possible cases that might happen.

1. One token being added twice in a Vault. Token would be counted doubly in the vault. Ref: Vault.sol#L293-L303. There would be two item in the array when querying manager.getTokens(address(this));.

2. A token first being added to two vaults. The value calculation of the first vault would be broken. As vaults[_token] = _vault; would point to the other vault.

Permission keys should always be treated cautiously. However, calling the same initialize function twice should not be able to destroy the vault. Also, as the protocol develops, there's likely that one token is supported in two vaults. The DAO may mistakenly add the same token twice. I consider this a high-risk issue.

Proof of Concept

Adding same token twice would not raise any error here.

manager.functions.addToken(vault.address, dai.address).transact()
manager.functions.addToken(vault.address, dai.address).transact()

Tools Used

Hardhat

Recommended Mitigation Steps

I recommend to add two checks

require(vaults[_token] == address(0));
bool notFound = True;
for(uint256 i; i < tokens[_vault].length; i++) {
    if (tokens[_vault] == _token) {
        notFound = False;
    }
}
require(notFound, "duplicate token");

Handle

jonah1005

Vulnerability details

Impact

When a user tries to withdraw the token from the vault, the vault would withdraw the token from the controller if there's insufficient liquidity in the vault. However, the controller does not raise an error when there's insufficient liquidity in the controller/ strategies. The user would lose his shares while getting nothing.

An MEV searcher could apply this attack on any withdrawal. When an attacker found an unconfirmed tx that tries to withdraw 1M dai, he can do such sandwich attack.

1. Deposits USDC into the vault.
2. Withdraw all dai left in the vault/controller/strategy.
3. Place the vitims tx here. The victim would get zero dai while burning 1 M share. This would pump the share price.
4. Withdraw all liquidity.

All users would be vulnerable to MEV attackers. I consider this is a high-risk issue.

Proof of Concept

Here's web3.py script to reproduce the issue.

deposit_amount = 100000 * 10**18
user = w3.eth.accounts[0]
get_token(dai, user, deposit_amount)
dai.functions.approve(vault.address, deposit_amount + margin_deposit).transact()
vault.functions.deposit(dai.address, deposit_amount).transact()
vault.functions.withdrawAll(usdt.address).transact()

#
print("usdt amount: ", usdt.functions.balanceOf(user).call())

Tools Used

None

Recommended Mitigation Steps

There are two issues involved. First, users pay the slippage when they try to withdraw. I do not find this fair. Users have to pay extra gas to withdraw liquidity from strategy, convert the token, and still paying the slippage. I recommend writing a view function for the frontend to display how much slippage the user has to pay. Controler.sol#L448-L479

Second, the controller does not revert the transaction there's insufficient liquidity. Controller.sol#L577-L622 Recommend to revert the transaction when _amount is not equal to zero after the loop finish.

Handle

jonah1005

Vulnerability details

Impact

The v3 vault treats all valid tokens exactly the same. Depositing 1M DAI would get the same share as depositing 1M USDT. User can withdraw their share in another token. Though there's withdrawalProtectionFee (0.1 percent), the vault is still a no slippage stable coin exchange.

Also, I notice that 3crv_token is added to the vault in the test. Treating 3crv_token and all other stable coins the same would make the vault vulnerable to flashloan attack. 3crv_token is an lp token and at the point of writing, the price of it is 1.01. The arbitrage space is about 0.8 percent and makes the vault vulnerable to flashloan attacks.

Though the team may not add crv_token and dai to the same vault, its design makes the vault vulnerable. Strategies need to be designed with super caution or the vault would be vulnerable to attackers.

Given the possibility of flashloan attack, I consider this a high-risk issue.

Proof of Concept

The issue locates at the deposit function. Vault.sol#L147-L180

The share is mint according to the calculation here

            _shares = _shares.add(_amount);

The share is burned at Vault.sol#L217

uint256 _amount = (balance().mul(_shares)).div(totalSupply());

Here's a sample exploit in web3.py.

deposit_amount = 100000 * 10**6
user = w3.eth.accounts[0]
get_token(usdt, user, deposit_amount)
usdt.functions.approve(vault.address, deposit_amount).transact()
vault.functions.deposit(usdt.address, deposit_amount).transact()
vault.functions.withdrawAll(t3crv.address).transact()
# user can remove liquiditiy and get the profit.

Tools Used

Hardhat

Recommended Mitigation Steps

Given the protocols' scenario, I feel like we can take iearn token's architect as a Ref. yDdai

yDai handles multiple tokens (cDai/ aDai/ dydx/ fulcrum). Though four tokens are pretty much the same, the contract still needs to calculate the price of each token.

Or, create a vault for each token might be an easier quick fix.

Handle

jonah1005

Vulnerability details

Impact

For a dai vault that pairs with NativeStrategyCurve3Crv, every time earn() is called, shareholders would lose money. (about 2%)

There're two issues involved. The Vault contract and the controller contract doesn't handle the price difference between the want token and other tokens.

Vault.sol#L293 When a vault calculates its value, it sums up all tokens balance. Controller.sol#L410-L436 However, when the controller calculates vaults' value, it only adds the amount of strategy.want it received. (in this case, it's t3crv).

Under the current design, users who deposit dai to the vault would not get yield. Instead, they would keep losing money. I consider this a high-risk issue

Proof of Concept

I trigger the bug with the following web3.py script:

previous_price = vault.functions.getPricePerFullShare().call()
vault.functions.available(dai.address).call()
vault.functions.earn(dai.address, strategy.address).transact()
current_price = vault.functions.getPricePerFullShare().call()
print(previous_price)
print(current_price)

Tools Used

Hardhat

Recommended Mitigation Steps

The protocol should decide what the balance sheet in each contract stands for and make it consistent in all cases. Take, for example, if _vaultDetails[_vault].balance; stands for the amount of 'want' token the vault owns, there shouldn't exist two different want in all the strategies the vault has. Also, when the vault queries controllers function balanceOf(), they should always multiply it by the price.

Handle

jonah1005

Vulnerability details

Impact

The vault does not normalize decimals when a user withdraws the token. When a user has 100e18 shares, he can withdraw all usdc/ usdt from the token.

The liquidity of USDC/USDC would be drained. I consider this is a high-risk issue.

Proof of Concept

Vault.sol#L232-L261

This is the web3.py script to drain all usdt from the vault.

usdt_balance = usdt.functions.balanceOf(vault.address).call()
print('previous usdt:', usdt_balance)
# We deposit some margin dai to drain all liquidity.
deposit_amount = usdt_balance + usdt_balance
dai.functions.approve(vault.address, deposit_amount).transact()
vault.functions.deposit(dai.address, deposit_amount).transact()
print('amount of share:', vault.functions.balanceOf(user).call())
vault.functions.withdrawAll(usdt.address).transact()

## usdt would left zero
print('usdt left:', usdt.functions.balanceOf(vault.address).call())

Tools Used

None

Recommended Mitigation Steps

Normalize decimals on withdrawing.

Handle

jonah1005

Vulnerability details

removeToken would break the vault.

Impact

There's no safety check in Manager.sol's removeToken. Manager.sol#L454-L487

1. The token would be locked in the original vault. Given the current design, the vault would keep a ratio of total amount to save the gas. Once the token is removed at manager contract, these token would lost.
2. Controller's balanceOf would no longer reflects the real value. Controller.sol#L488-L495 While _vaultDetails[msg.sender].balance; remains the same, user can nolonger withdraw those amount.
3. Share price in the vault would decrease drastically. The share price is calculated as totalValue / totalSupply Vault.sol#L217. While the totalSupply of the share remains the same, the total balance has drastically decreased.

Calling removeToken way would almost break the whole protocol if the vault has already started. I consider this is a high-risk issue.

Proof of Concept

We can see how the vault would be affected with below web3.py script.

print(vault.functions.balanceOfThis().call())
print(vault.functions.totalSupply().call())
manager.functions.removeToken(vault.address, dai.address).transact()
print(vault.functions.balanceOfThis().call())
print(vault.functions.totalSupply().call())

output

100000000000000000000000
100000000000000000000000
0
100000000000000000000000

Tools Used

Hardhat

Recommended Mitigation Steps

Remove tokens from a vault would be a really critical job. I recommend the team cover all possible cases and check all components' states (all vault/ strategy/ controller's state) in the test.

Some steps that I try to come up with that is required to remove TokenA from a vault.

1. Withdraw all tokenA from all strategies (and handle it correctly in the controller).
2. Withdraw all tokenA from the vault.
3. Convert all tokenA that's collected in the previous step into tokenB.
4. Transfer tokenB to the vault and compensate the transaction fee/slippage cost to the vault.

Handle

jonah1005

Vulnerability details

Impact

The Controller's function withdraw(address _token, uint256 _amount) should return whatever amount of the token user/vault asks. However, it tries to withdraw strategy.want token and convert it.

Take for example, when a user/vault calls withdraw(dai, 100), the controller should transfer 100 dai to the user/vault; instead, it withdraw 100 t3crv from the strategy, convert it to dai, and transfer the converted amount to the user. In this case, the user would get about 101 dai. (1 t3crv = 1.01 dai).

1 percent of arbitrage space is enough to cause painful results. Also, the arbitrage space really depends on the strategy. Strategy should be able to have whatever want token, e.g., ETH, CRV,.... Vault users would definetly lose their money if the price not being handled properly.

I consider this a high-risk issue.

Proof of Concept

Controller.sol#L446-L477

We can trigger the bug with following web3.py script.

1. Two users deposit to the vault.
2. Admin call earn
3. Two users withdraw all from the vault.

deposit_amount = 100000 * 10**18
user = w3.eth.accounts[0]
get_token(dai, user, deposit_amount)
dai.functions.approve(vault.address, deposit_amount).transact()
vault.functions.deposit(dai.address, deposit_amount).transact()

deposit_amount = 100000 * 10**18
user2 = w3.eth.accounts[1]
get_token(dai, user2, deposit_amount)
dai.functions.approve(vault.address, deposit_amount).transact({'from': user2})
vault.functions.deposit(dai.address, deposit_amount).transact({'from': user2})
vault.functions.earn(dai.address, strategy.address).transact()
vault.functions.withdrawAll(dai.address).transact({'from': user})
## raise error here
vault.functions.withdrawAll(dai.address).transact({'from': user2})

Since the first user gets more tokens than he should have, user2 would not be able to withdraw all the shares.

Tools Used

None

Recommended Mitigation Steps

Handle

jonah1005

Vulnerability details

manager does not use safe transfer

Impact

Manager contract does not use safeTransfer in function recoverToken. Mansger.sol#L442-L452 Manager contract would not be able to transfer non-standard ERC20 tokens.

Given the manager is not designed to hold the token, I consider this a low-risk issue.

Proof of Concept

Mansger.sol#L442-L452

Tools Used

None

Recommended Mitigation Steps

I recommend using safeERC20 as other contracts do.

Handle

jonah1005

Vulnerability details

Impact

The protocol frequently interacts with crv a lot. However, the contract doesn't specify the minimum return amount. Given the fact that there's a lot of MEV searchers, calling swap without specifying the minimum return amount really puts user funds in danger.

For example, controller's withdrawAll is designed to transfer all the funds in a strategy.Controller.sol#L360 The arbitrage space is enough for a searcher to sandwich this trade.

Proof of Concept

Mansger.sol#L442-L452

Controller.sol#L273

Tools Used

None

Recommended Mitigation Steps

Always calculates an estimated return when calling to crv.