diff --git a/src/bridges/liquity/TroveBridge.sol b/src/bridges/liquity/TroveBridge.sol index d95f047d2..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) @@ -471,11 +475,17 @@ 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; } + // 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 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( abi.encodeWithSignature( "swap(address,bool,int256,uint160,bytes)", @@ -489,13 +499,13 @@ 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 - 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 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();