Controller transfer extra token on withdrawing tokens

[code423n4]

Handle

jonah1005

Vulnerability details

Impact

The Controlle�r'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

[gpersoon]

I think this is related to the underlying problem that all coins are assumed to have the same value.
See also #2 and #158

[GalloDaSballo]

Duplicate of #77