Vault multihop swaps for any token pair will fail in one direction

Summary

The Unstoppable SwapRouter's add_path() function stores the same swap path for swapping from "token1" to "token2" and from "token2" to "token1". If the swap path starts with the token1 address, swaps from token2 to token1 will fail, and vice versa.

Vulnerability Detail

The administrator must call SwapRouter add_path() to add support for swaps that use multiple Uniswap pools (multihop swaps). The path provided is a sequence of token addresses and pool fee amounts. The path is stored in the self.paths hashmap using the provided token1 and token2 addresses as the keys. The path is stored twice, once for each ordering of those keys such that the stored path can be accessed easily without sorting the keys.

When a path stored in the Unstoppable Vault is used for a swap, it is obtained by indexing the hashmap first with the address of the input token for the swap:

    path: Bytes[66] = self.paths[_token_in][_token_out]

But, because of the logic in add_path() described above, the first address in the obtained path could be either _token_in or _token_out. In the case that it is _token_out, the swap will fail as described below.

The stored path is passed along for use by the Uniswap V3 SwapRouter which assumes that the first 2 addresses in the path define the first pool to use. The SwapRouter uses Path.decodeFirstPool() to extract the first address, and it is the first return value thus interpreted as tokenIn for the swap:

        (address tokenIn, address tokenOut, uint24 fee) = data.path.decodeFirstPool();

In the case that the first token address in the path is actually the intended output token for this swap, the Uniswap V3 SwapRouter will attempt to transfer the input amount of the output token and fail because it does not have any allowance to access the Unstoppable SwapRouter contract's balance of the output token.

Impact

The planned margin trading functionality will not work as intended when multihop swaps are required between the margin/debt token and the position token. Since such swaps will work in one direction but fail in the other, traders would either be unable to obtain the desired position or, in the worst case, be unable to exit the position and recover their margin funds. Note that in the latter case, liquidators will also be unable to force exit of over-leveraged positions.

Code Snippet

SwapRouter.add_path() at https://github.com/Unstoppable-DeFi/unstoppable-dex-audit/blob/4153c3e67ccc080032ba0bbaffd9a0c56a573070/contracts/margin-dex/SwapRouter.vy#L130-L134

@external
def add_path(_token1: address, _token2: address, _path: Bytes[66]):
    assert msg.sender == self.admin, "unauthorized"
    self.paths[_token1][_token2] = _path
    self.paths[_token2][_token1] = _path

Tool used

Manual Review

Recommendation

Modify the SwapRouter add_path() function: remove the _token1 and _token2 parameters and instead infer them from the contents of _path. Make a reversed copy of the _path and store both versions in the paths hashmap with key ordering that matches the path being stored. Add Vault tests for multihop swaps.

141345 commented 1 year ago

This one will inevitably result in user loss, high severity might be more appropriate.

twicek commented 1 year ago

Escalate for 10 USDC. Assuming this is true: transfer the input amount of the output token. There is no allowance given for the output token, it will always revert as said in the report. If someone can find an example where it leads to loss of funds I agree that it should be high severity, otherwise it should be medium severity.

sherlock-admin2 commented 1 year ago

Escalate for 10 USDC. Assuming this is true: transfer the input amount of the output token. There is no allowance given for the output token, it will always revert as said in the report. If someone can find an example where it leads to loss of funds I agree that it should be high severity, otherwise it should be medium severity.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

141345 commented 1 year ago

Recommendation: change the original judging, to medium

Escalate for 10 USDC. Assuming this is true: transfer the input amount of the output token. There is no allowance given for the output token, it will always revert as said in the report. If someone can find an example where it leads to loss of funds I agree that it should be high severity, otherwise it should be medium severity.

The mentioned case is only 1 scenario. The possible impactful scenario is, token1 is transferred in. But when close the position, the reverse process will revert, causing user fund locked.

Then the admin can use add_path to rescue the funds by changing the path.

Based on the above, I think medium is appropriate.