Skip to content

Commit

Permalink
Merge pull request #31 from Blueberryfi/sherlock-audit-fix-132
Browse files Browse the repository at this point in the history 
fix: issue#132 - UniswapV3 sqrtRatioLimit doesn't provide slippage protection and will result in partial swaps
  • Loading branch information
@Gornutz
Gornutz authored May 29, 2023
2 parents e7bbd70 + bd45e1d commit 314eaf2
Show file tree
Hide file tree
Showing 3 changed files with 102 additions and 7 deletions.
46 changes: 41 additions & 5 deletions contracts/spell/IchiSpell.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,32 +209,39 @@ contract IchiSpell is BasicSpell, IUniswapV3SwapCallback {

// 4. Swap withdrawn tokens to debt token
bool isTokenA = vault.token0() == param.borrowToken;
uint256 amountToSwap = IERC20Upgradeable(
uint256 amountIn = IERC20Upgradeable(
isTokenA ? vault.token1() : vault.token0()
).balanceOf(address(this));

if (amountToSwap > 0) {
if (amountIn > 0) {
uint160 deltaSqrt = (param.sqrtRatioLimit *
uint160(param.sellSlippage)) / uint160(Constants.DENOMINATOR);
address[] memory swapPath = new address[](2);
swapPath[0] = isTokenA ? vault.token1() : vault.token0();
swapPath[1] = isTokenA ? vault.token0() : vault.token1();

uint256 amountOutMin = _getMinOutAmount(
swapPath[0],
swapPath[1],
amountIn,
param.sellSlippage
);

IUniswapV3Router.ExactInputSingleParams
memory params = IUniswapV3Router.ExactInputSingleParams({
tokenIn: swapPath[0],
tokenOut: swapPath[1],
fee: IUniswapV3Pool(vault.pool()).fee(),
recipient: address(this),
deadline: block.timestamp + Constants.MAX_DELAY_ON_SWAP,
amountIn: amountToSwap,
amountOutMinimum: 0,
amountIn: amountIn,
amountOutMinimum: amountOutMin,
sqrtPriceLimitX96: isTokenA
? param.sqrtRatioLimit + deltaSqrt

This comment has been minimized.

Sign in to view

Copy link
@IAm0x52

IAm0x52 Jun 16, 2023

Collaborator

No need to specify any sqrtPriceLimitX96 here at all, just use 0 instead. amountOutMin is already giving slippage protection so it's not longer necessary to specify this.
: param.sqrtRatioLimit - deltaSqrt
});

_ensureApprove(params.tokenIn, address(uniV3Router), amountToSwap);
_ensureApprove(params.tokenIn, address(uniV3Router), amountIn);
uniV3Router.exactInputSingle(params);
}

Expand All @@ -251,6 +258,35 @@ contract IchiSpell is BasicSpell, IUniswapV3SwapCallback {
_doRefund(param.collToken);
}

/**
* @notice Internal function to get outToken amount based on inToken amount
* @param inToken Address of inToken
* @param outToken Address of outToken
* @param amountIn Amount of inToken
* @param slippage Swap slippage
*/
function _getMinOutAmount(
address inToken,
address outToken,
uint256 amountIn,
uint256 slippage
) internal returns (uint256) {
uint256 inTokenPrice = bank.oracle().getPrice(inToken);
uint256 outTokenPrice = bank.oracle().getPrice(outToken);
uint256 inTokenDecimal = IERC20MetadataUpgradeable(inToken).decimals();
uint256 outTokenDecimal = IERC20MetadataUpgradeable(outToken)
.decimals();

// calculate amountOut based on this formular. token0 blance * token0 price = token1 balance * token1 price
// decimal should be considered
uint256 amountOut = ((amountIn * inTokenPrice) *
(10 ** outTokenDecimal)) / (outTokenPrice * (10 ** inTokenDecimal));

return
(amountOut * (Constants.DENOMINATOR - slippage)) /
Constants.DENOMINATOR;
}

/**
* @notice External function to withdraw assets from ICHI Vault
*/
Expand Down
26 changes: 26 additions & 0 deletions test/bank.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,12 @@ describe('Bank', () => {
await bank.whitelistERC1155([wichi.address], true);
})
it("should revert close execution for existing position with different isolated token", async () => {
await mockOracle.setPrice(
[ICHI],
[
BigNumber.from(10).pow(16).mul(326), // $3.26
]
);
await bank.execute(
0,
spell.address,
Expand Down Expand Up @@ -421,6 +427,13 @@ describe('Bank', () => {
const tick = await ichiVault.currentTick();
const sqrt = TickMath.getSqrtRatioAtTick(tick);
await usdc.approve(bank.address, ethers.constants.MaxUint256);
await mockOracle.setPrice(
[ICHI],
[
BigNumber.from(10).pow(16).mul(350), // $3.5
]
);

await expect(
bank.execute(
positionId,
Expand All @@ -439,6 +452,12 @@ describe('Bank', () => {
).to.be.revertedWith("INCORRECT_DEBT")
})
it("should revert close execution for for not whitelisted debt token", async () => {
await mockOracle.setPrice(
[ICHI],
[
BigNumber.from(10).pow(16).mul(326), // $3.26
]
);
await bank.execute(
0,
spell.address,
Expand Down Expand Up @@ -902,6 +921,13 @@ describe('Bank', () => {
const tick = await ichiVault.currentTick();
const sqrt = TickMath.getSqrtRatioAtTick(tick);
await ichi.approve(bank.address, ethers.constants.MaxUint256);

await mockOracle.setPrice(
[ICHI],
[
BigNumber.from(10).pow(16).mul(326), // $3.26
]
);
await expect(
bank.execute(
positionId,
Expand Down
37 changes: 35 additions & 2 deletions test/spell/ichi.spell.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ describe('ICHI Angel Vaults Spell', () => {
[USDC, ICHI],
[
BigNumber.from(10).pow(18), // $1
BigNumber.from(10).pow(17).mul(40), // $4
BigNumber.from(10).pow(16).mul(400), // $4
]
);
risk = await bank.callStatic.getPositionRisk(1);
Expand Down Expand Up @@ -414,7 +414,39 @@ describe('ICHI Angel Vaults Spell', () => {
)
).to.be.revertedWith("RATIO_TOO_HIGH")
})
it("should revert when token price is changed and outToken amount is out of slippage range", async () => {
const positionId = (await bank.nextPositionId()).sub(1);
const positionInfo = await bank.getPositionInfo(positionId);
await ichiVault.rebalance(-260400, -260200, -260800, -260600, 0);
await usdc.transfer(spell.address, utils.parseUnits('10', 6)); // manually set rewards

const tick = await ichiVault.currentTick();
const sqrt = TickMath.getSqrtRatioAtTick(tick);
await expect(
bank.execute(
positionId,
spell.address,
iface.encodeFunctionData("closePosition", [{
strategyId: 0,
collToken: ICHI,
borrowToken: USDC,
amountRepay: 0,
amountPosRemove: positionInfo.collateralSize.div(3),
amountShareWithdraw: 0,
sellSlippage: 50,
sqrtRatioLimit: BigNumber.from(sqrt.toString())
}])
)
).to.be.revertedWith("Too little received");
})
it("should be able to close portion of position without withdrawing isolated collaterals", async () => {
await mockOracle.setPrice(
[USDC, ICHI],
[
BigNumber.from(10).pow(18), // $1
BigNumber.from(10).pow(16).mul(326), // $3.26
]
);
const positionId = (await bank.nextPositionId()).sub(1);
const positionInfo = await bank.getPositionInfo(positionId);
await ichiVault.rebalance(-260400, -260200, -260800, -260600, 0);
Expand Down Expand Up @@ -686,7 +718,7 @@ describe('ICHI Angel Vaults Spell', () => {
[USDC, ICHI],
[
BigNumber.from(10).pow(18), // $1
BigNumber.from(10).pow(17).mul(40), // $4
BigNumber.from(10).pow(16).mul(326), // $3.26
]
);
risk = await bank.callStatic.getPositionRisk(2);
Expand Down Expand Up @@ -853,6 +885,7 @@ describe('ICHI Angel Vaults Spell', () => {
}])
);
})

it("should revert maintaining position when farming pool id does not match", async () => {
const nextPosId = await bank.nextPositionId();
await expect(
Expand Down

0 comments on commit 314eaf2

Please sign in to comment.