search

Fujicracy / fuji-v2

Cross-chain money market aggregator
https://fuji-v2-frontend.vercel.app
15 stars 10 forks source link

Protocol/fix/cs swapper slippage too high #252

Closed 0xdcota closed 1 year ago

0xdcota commented 1 year ago

This PR should be merged after 225.

0xdcota commented 1 year ago

@brozorec this is the last of the CS - security audit findings. I implemented 3 simple tests for the swapper.

In the process of fixing the swapper, I thought that we should implement quoting methods, wdyt?

I implemented these compute_amountOut and compute_amountIn in the test file, but I actually think this should be part of the Swapper interface itself.

https://github.com/Fujicracy/fuji-v2/blob/44f3cbd919005a9cebc13978a5ab271d549efeb0/packages/protocol/test/forking/mainnet/UniswapV2Swapper.t.sol#L84-L115

In addition, I propose we make an additional ticket-issue to make the swapper more secure against slippage. This is by checking the price against an external oracle. We did something similar on V1.

https://github.com/Fujicracy/fuji-v1/blob/c040d6407111be1a48e54fb847856fe2ce80ff6a/packages/hardhat/contracts/mainnet/Fliquidator.sol#L461-L476

brozorec commented 1 year ago

In the process of fixing the swapper, I thought that we should implement quoting methods, wdyt?

I implemented these compute_amountOut and compute_amountIn in the test file, but I actually think this should be part of the Swapper interface itself.

@DaigaroCota I agree but let's call them getAmountIn and getAmountOut :)

In addition, I propose we make an additional ticket-issue to make the swapper more secure against slippage. This is by checking the price against an external oracle. We did something similar on V1.

a very good idea :+1:

0xdcota commented 1 year ago

@brozorec ready for review.

Additional issue created to add external oracle check; refer to #255 .

0xdcota commented 1 year ago

@ComposableSecurityTeam this is 2/2 missing PRs to complete Audit findings (excluding informational).