From 89129e99529b0095310d52396a68edf251043a9b Mon Sep 17 00:00:00 2001 From: benesjan Date: Fri, 18 Nov 2022 12:15:41 -0600 Subject: [PATCH 1/5] fix: critical underflow vulnerability fix --- src/bridges/liquity/TroveBridge.sol | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/bridges/liquity/TroveBridge.sol b/src/bridges/liquity/TroveBridge.sol index d95f047d2..db4179b65 100644 --- a/src/bridges/liquity/TroveBridge.sol +++ b/src/bridges/liquity/TroveBridge.sol @@ -476,6 +476,11 @@ contract TroveBridge is BridgeBase, ERC20, Ownable, IUniswapV3SwapCallback { lusdToBuy = debtToRepay; } + // Saving a balance to a local variable to later get a `collateralReturned` value unaffected by a previously + // held balance --> This is important because if we set `collateralReturned` to `address(this).balance` + // the value might be larger than `collToWithdraw` which could cause underflow when computing `collateralSold` + uint256 ethBalanceBeforeSwap = address(this).balance; + (bool success, ) = LUSD_USDC_POOL.call( abi.encodeWithSignature( "swap(address,bool,int256,uint160,bytes)", @@ -489,7 +494,7 @@ contract TroveBridge is BridgeBase, ERC20, Ownable, IUniswapV3SwapCallback { if (success) { // Note: Debt repayment took place in the `uniswapV3SwapCallback(...)` function - collateralReturned = address(this).balance; + collateralReturned = address(this).balance - ethBalanceBeforeSwap; { // Check that at most `maxCost` of ETH collateral was sold for `debtToRepay` worth of LUSD From 1bc1fde2a395a4bb6c7d2e67640e3870900fb646 Mon Sep 17 00:00:00 2001 From: benesjan Date: Fri, 18 Nov 2022 12:24:42 -0600 Subject: [PATCH 2/5] refactor: improved naming --- src/bridges/liquity/TroveBridge.sol | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/bridges/liquity/TroveBridge.sol b/src/bridges/liquity/TroveBridge.sol index db4179b65..6fd290ee7 100644 --- a/src/bridges/liquity/TroveBridge.sol +++ b/src/bridges/liquity/TroveBridge.sol @@ -477,8 +477,9 @@ contract TroveBridge is BridgeBase, ERC20, Ownable, IUniswapV3SwapCallback { } // Saving a balance to a local variable to later get a `collateralReturned` value unaffected by a previously - // held balance --> This is important because if we set `collateralReturned` to `address(this).balance` - // the value might be larger than `collToWithdraw` which could cause underflow when computing `collateralSold` + // held balance --> This is important because if we would set `collateralReturned` to `address(this).balance` + // the value might be larger than `collToWithdraw` which could cause underflow when computing + // `collateralSoldToUniswap` uint256 ethBalanceBeforeSwap = address(this).balance; (bool success, ) = LUSD_USDC_POOL.call( @@ -497,10 +498,10 @@ contract TroveBridge is BridgeBase, ERC20, Ownable, IUniswapV3SwapCallback { collateralReturned = address(this).balance - ethBalanceBeforeSwap; { - // Check that at most `maxCost` of ETH collateral was sold for `debtToRepay` worth of LUSD - uint256 maxCost = (lusdToBuy * _maxPrice) / PRECISION; - uint256 collateralSold = collToWithdraw - collateralReturned; - if (collateralSold > maxCost) revert MaxCostExceeded(); + // Check that at most `maxCostInETH` of ETH collateral was sold for `debtToRepay` worth of LUSD + uint256 maxCostInETH = (lusdToBuy * _maxPrice) / PRECISION; + uint256 collateralSoldToUniswap = collToWithdraw - collateralReturned; + if (collateralSoldToUniswap > maxCostInETH) revert MaxCostExceeded(); } // Burn all input TB From 732c0e6339702f718f309a449f37ac8b146d124b Mon Sep 17 00:00:00 2001 From: benesjan Date: Fri, 18 Nov 2022 12:56:17 -0600 Subject: [PATCH 3/5] fix: overshadow issue --- src/bridges/liquity/TroveBridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bridges/liquity/TroveBridge.sol b/src/bridges/liquity/TroveBridge.sol index 6fd290ee7..3c6376d73 100644 --- a/src/bridges/liquity/TroveBridge.sol +++ b/src/bridges/liquity/TroveBridge.sol @@ -471,7 +471,7 @@ contract TroveBridge is BridgeBase, ERC20, Ownable, IUniswapV3SwapCallback { // Reverting here because an incorrect flow has been chosen --> there is no reason to be using flash swaps // when the amount of LUSD on input is enough to cover the debt if (debtToRepay <= _totalInputValue) revert ErrorLib.InvalidOutputB(); - uint256 lusdToBuy = debtToRepay - _totalInputValue; + lusdToBuy = debtToRepay - _totalInputValue; } else { lusdToBuy = debtToRepay; } From ad5a82486d80c7d55c13cfff4a43f2c5929bd9f4 Mon Sep 17 00:00:00 2001 From: benesjan Date: Mon, 21 Nov 2022 09:16:02 -0600 Subject: [PATCH 4/5] test: trove underflow bug --- .../bridges/liquity/TroveBridgeUnit.t.sol | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/test/bridges/liquity/TroveBridgeUnit.t.sol b/src/test/bridges/liquity/TroveBridgeUnit.t.sol index 3310738af..b03c3dbef 100644 --- a/src/test/bridges/liquity/TroveBridgeUnit.t.sol +++ b/src/test/bridges/liquity/TroveBridgeUnit.t.sol @@ -350,6 +350,38 @@ contract TroveBridgeUnitTest is TroveBridgeTestBase { _openTrove(); } + // @dev run against a block when the flash swap doesn't fail + function testRedistributionUnderflowBug() public { + _setUpRedistribution(); + + // Mint ETH to the bridge to test whether the underflow bug found in the 2nd audit is still present + deal(address(bridge), 100e18); + + // Repay + AztecTypes.AztecAsset memory tbAsset = AztecTypes.AztecAsset( + 2, + address(bridge), + AztecTypes.AztecAssetType.ERC20 + ); + AztecTypes.AztecAsset memory lusdAsset = AztecTypes.AztecAsset( + 1, + tokens["LUSD"].addr, + AztecTypes.AztecAssetType.ERC20 + ); + AztecTypes.AztecAsset memory ethAsset = AztecTypes.AztecAsset(3, address(0), AztecTypes.AztecAssetType.ETH); + + // inputValue is equal to rollupProcessor TB balance --> we want to repay the debt in full + uint256 inputValue = bridge.balanceOf(rollupProcessor); + // Transfer TB to the bridge + IERC20(tbAsset.erc20Address).transfer(address(bridge), inputValue); + + // Mint the amount to repay to the bridge + deal(lusdAsset.erc20Address, address(bridge), inputValue + bridge.DUST()); + + // If the bug is present the following will revert with underflow + bridge.convert(tbAsset, lusdAsset, ethAsset, tbAsset, inputValue, 1, _getPrice(-1e20), address(0)); + } + function testRepayingAfterRedistributionRevertsWhenMaxCostExceeded() public { _setUpRedistribution(); From a84f48a988fcbdda98a586f84b7248a54bada6a3 Mon Sep 17 00:00:00 2001 From: benesjan Date: Mon, 21 Nov 2022 13:33:25 -0600 Subject: [PATCH 5/5] feat: making input checks more explicit --- src/bridges/liquity/TroveBridge.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/bridges/liquity/TroveBridge.sol b/src/bridges/liquity/TroveBridge.sol index 3c6376d73..81e519440 100644 --- a/src/bridges/liquity/TroveBridge.sol +++ b/src/bridges/liquity/TroveBridge.sol @@ -214,6 +214,7 @@ contract TroveBridge is BridgeBase, ERC20, Ownable, IUniswapV3SwapCallback { if ( _inputAssetA.assetType == AztecTypes.AztecAssetType.ETH && + _inputAssetB.assetType == AztecTypes.AztecAssetType.NOT_USED && _outputAssetA.erc20Address == address(this) && _outputAssetB.erc20Address == LUSD ) { @@ -248,7 +249,10 @@ contract TroveBridge is BridgeBase, ERC20, Ownable, IUniswapV3SwapCallback { } subsidyCriteria = 1; } else if ( - _inputAssetA.erc20Address == address(this) && _outputAssetA.assetType == AztecTypes.AztecAssetType.ETH + _inputAssetA.erc20Address == address(this) && + _inputAssetB.assetType == AztecTypes.AztecAssetType.NOT_USED && + _outputAssetA.assetType == AztecTypes.AztecAssetType.ETH && + _outputAssetB.assetType == AztecTypes.AztecAssetType.NOT_USED ) { if (troveStatus == Status.active) { // Repaying debt with collateral (using flash swaps)