Merge pull request #284 from AztecProtocol/janb/second-trove-audit-fe…

…edback
AztecProtocol · Nov 21, 2022 · c2b4904 · c2b4904
2 parents ee5fbe2 + a84f48a
commit c2b4904
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 7 deletions.
diff --git 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
@@ -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();