diff --git a/contracts/BorrowerOperations.sol b/contracts/BorrowerOperations.sol index 8043271..c80a9f7 100644 --- a/contracts/BorrowerOperations.sol +++ b/contracts/BorrowerOperations.sol @@ -1,5 +1,4 @@ // SPDX-License-Identifier: MIT - pragma solidity 0.6.11; pragma experimental ABIEncoderV2; @@ -159,6 +158,25 @@ contract BorrowerOperations is emit ZEROStakingAddressChanged(_zeroStakingAddress); } + /* + * @notice set the user's block number for opening or increasing LoCs, and do check & reset to 0 for closing or decreasing + * + * @dev This is the function will be called by the open, close, increase, decrease trove function + */ + function recoveryModeMutexHandler(bool _openOrIncrease) private { + if(_openOrIncrease) { + recoveryModeMutex[msg.sender] = block.number; + } else { + if(recoveryModeMutex[msg.sender] > 0) { + if(recoveryModeMutex[msg.sender] == block.number) { + revert("Recovery mode mutex locked. Try in another block"); + } + + recoveryModeMutex[msg.sender] = 0; + } + } + } + function setMassetManagerAddress(address _massetManagerAddress) external onlyOwner { massetManager = IMassetManager(_massetManagerAddress); emit MassetManagerAddressChanged(_massetManagerAddress); @@ -203,6 +221,10 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); bool isRecoveryMode = _checkRecoveryMode(vars.price); + if(isRecoveryMode && _ZUSDAmount > 0) { + recoveryModeMutexHandler(true); + } + _requireValidMaxFeePercentage(_maxFeePercentage, isRecoveryMode); _requireTroveisNotActive(contractsCache.troveManager, msg.sender); @@ -585,6 +607,10 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); vars.isRecoveryMode = _checkRecoveryMode(vars.price); + if(vars.isRecoveryMode && _ZUSDChange > 0) { + recoveryModeMutexHandler(_isDebtIncrease); + } + if (_isDebtIncrease) { _requireValidMaxFeePercentage(_maxFeePercentage, vars.isRecoveryMode); _requireNonZeroDebtChange(_ZUSDChange); diff --git a/contracts/BorrowerOperationsStorage.sol b/contracts/BorrowerOperationsStorage.sol index 9e7d769..658fa10 100644 --- a/contracts/BorrowerOperationsStorage.sol +++ b/contracts/BorrowerOperationsStorage.sol @@ -36,4 +36,9 @@ contract BorrowerOperationsStorage is Ownable { IMassetManager public massetManager; IFeeDistributor public feeDistributor; + + /* + * Store the LoC block number on open/increase when in the Recovery mode + */ + mapping(address => uint256) public recoveryModeMutex; } diff --git a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol new file mode 100644 index 0000000..fdd3f4d --- /dev/null +++ b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.6.11; + +import "../Interfaces/IBorrowerOperations.sol"; +import "hardhat/console.sol"; + +interface IPriceFeedTestnet { + function setPrice(uint256 price) external returns (bool); +} + +contract BorrowerOperationsCrossReentrancy { + IBorrowerOperations public borrowerOperations; + + constructor( + IBorrowerOperations _borrowerOperations + ) public { + borrowerOperations = _borrowerOperations; + } + + fallback() external payable {} + + function testCrossReentrancyWithoutAffectingDebt( + uint256 _maxFeePercentage, + uint256 _ZUSDAmount, + address _upperHint, + address _lowerHint, + address _priceFeed + ) public payable { + borrowerOperations.openTrove{value: msg.value / 2}( + _maxFeePercentage, + _ZUSDAmount, + _upperHint, + _lowerHint + ); + + // manipulate the price so that the recovery mode will be triggered + IPriceFeedTestnet(_priceFeed).setPrice(1e8); + + // // should not revert because it's not affecting the debt + borrowerOperations.addColl{value: msg.value / 2}(_upperHint, _lowerHint); + } + + /** + * @dev open trove and decrease the debt in the same block should revert due to reentrancy + */ + function testCrossReentrancyAffectingDebt( + uint256 _maxFeePercentage, + uint256 _ZUSDAmount, + address _upperHint, + address _lowerHint, + address _priceFeed + ) public payable { + borrowerOperations.openTrove{value: msg.value}( + _maxFeePercentage, + _ZUSDAmount, + _upperHint, + _lowerHint + ); + + // manipulate the price so that the recovery mode will be triggered + IPriceFeedTestnet(_priceFeed).setPrice(1e8); + + // repayZusd will affect(decrease) the debt, should revert due to reentrancy violation + borrowerOperations.repayZUSD(_ZUSDAmount, _upperHint, _lowerHint); + } + + /** + * @dev increase the debt, then decrease the debt in the same block should revert due to reentrancy + * @dev the trove will be opened in the past block + */ + function testCrossReentrancyByIncreasingDebt( + uint256 _maxFeePercentage, + uint256 _ZUSDAmount, + address _upperHint, + address _lowerHint, + address _priceFeed + ) public payable { + // manipulate the price so that the recovery mode will be triggered + IPriceFeedTestnet(_priceFeed).setPrice(1e17); + // add coll to the trove so that the TCR will be above CCR + borrowerOperations.addColl{value: msg.value}(_upperHint, _lowerHint); + + // withdraw ZUSD to increase the debt + borrowerOperations.withdrawZUSD(_maxFeePercentage, _ZUSDAmount, _upperHint, _lowerHint); + + // repayZusd will affect(decrease) the debt, should revert due to reentrancy violation + borrowerOperations.repayZUSD(_ZUSDAmount, _upperHint, _lowerHint); + } +} diff --git a/package.json b/package.json index 994bdad..1c49ecf 100644 --- a/package.json +++ b/package.json @@ -25,7 +25,7 @@ "prepare:set-version": "node scripts/set-version.js", "prepare:dist": "yarn prepare-export-deployments && yarn prepare:set-version && yarn prepare-dist && yarn prepare-artifacts", "prepublishOnly": "yarn docgen", - "postinstall": "patch-package", + "postinstall": "yarn patch-package", "prepare-export-deployments": "yarn hardhat export --export rskSovrynMainnetDeployments.json --network rskSovrynMainnet && yarn hardhat export --export rskSovrynTestnetDeployments.json --network rskSovrynTestnet", "prepare-dist": "rm -rf ./dist && mkdir -p dist/contracts && rsync -a -m --exclude 'ZERO' --exclude 'TestContracts' --exclude '*/*ZERO*.sol' --exclude '*/*Community*' --exclude '*/permit2/' --include '*/' --include '*.sol' --exclude '*' 'contracts/' 'dist/contracts' && cp -R LICENSE docs 'artifacts/version' ./dist && mkdir -p dist/deployment && cp -R 'deployment/deployments/rskSovrynMainnet' 'deployment/deployments/rskSovrynTestnet' dist/deployment && cp ./README.md ./dist/README.md && mv rskSovrynTestnetDeployments.json rskSovrynMainnetDeployments.json ./dist/deployment && cp ./package.dist.json ./dist/package.json && cp ./index.dist.js ./dist/index.js", "dist-pack": "cd ./dist && yarn pack --filename ../sovryn-zero-contracts-package.tgz", @@ -66,7 +66,7 @@ "devDependencies": { "@ethersproject/abi": "^5.7.0", "@ethersproject/providers": "^5.7.2", - "@nomicfoundation/hardhat-chai-matchers": "^2.0.2", + "@nomicfoundation/hardhat-chai-matchers": "^2.0.2", "@nomicfoundation/hardhat-network-helpers": "^1.0.9", "@nomicfoundation/hardhat-toolbox": "^3.0.0", "@nomicfoundation/hardhat-verify": "^1.1.1", @@ -106,4 +106,4 @@ "typescript": "^4.9.5", "web3": "^1.3.4" } -} +} \ No newline at end of file diff --git a/tests/js/BorrowerOperationsTest.js b/tests/js/BorrowerOperationsTest.js index 0fbb096..17f1f72 100644 --- a/tests/js/BorrowerOperationsTest.js +++ b/tests/js/BorrowerOperationsTest.js @@ -10,6 +10,7 @@ const TroveManagerTester = artifacts.require("TroveManagerTester"); const ZUSDTokenTester = artifacts.require("./ZUSDTokenTester"); const MassetManagerTester = artifacts.require("MassetManagerTester"); const NueMockToken = artifacts.require("NueMockToken"); +const BorrowerOperationsCrossReentrancy = artifacts.require("BorrowerOperationsCrossReentrancy"); const { AllowanceProvider, PermitTransferFrom, SignatureTransfer } = require("@uniswap/permit2-sdk"); const th = testHelpers.TestHelper; @@ -6995,6 +6996,104 @@ contract("BorrowerOperations", async accounts => { assert.isTrue(newTCR.eq(expectedTCR)); }); + + it("openTrove(): open a Trove, then withdraw collateral will not revert because it's not affecting the debt", async () => { + await openTrove({ ICR: toBN(dec(2, 18)), extraParams: { from: alice } }); + await priceFeed.setPrice("105000000000000000000"); + + assert.isTrue(await th.checkRecoveryMode(contracts)); + + const extraZUSDAmount = toBN(dec(1000, 18)); + const MIN_DEBT = ( + await th.getNetBorrowingAmount(contracts, await contracts.borrowerOperations.MIN_NET_DEBT()) + ).add(th.toBN(1)); // add 1 to avoid rounding issues + const zusdAmount = MIN_DEBT.add(extraZUSDAmount); + + const price = await contracts.priceFeedTestnet.getPrice(); + const totalDebt = await th.getOpenTroveTotalDebt(contracts, zusdAmount); + const ICR = toBN(dec(2, 18)); + const borrowerOperationsCrossReentrancy = await BorrowerOperationsCrossReentrancy.new(contracts.borrowerOperations.address); + + await borrowerOperationsCrossReentrancy.testCrossReentrancyWithoutAffectingDebt( + th._100pct, + extraZUSDAmount, + whale, + whale, + priceFeed.address, + {value: ICR.mul(totalDebt).div(price).mul(toBN(2))} + ); + }); + + it("openTrove(): open a Trove, then adjust it in the same block should revert due to reentrancy", async () => { + await openTrove({ ICR: toBN(dec(2, 18)), extraParams: { from: alice } }); + await priceFeed.setPrice("105000000000000000000"); + + assert.isTrue(await th.checkRecoveryMode(contracts)); + + const extraZUSDAmount = toBN(dec(1000, 18)); + const MIN_DEBT = ( + await th.getNetBorrowingAmount(contracts, await contracts.borrowerOperations.MIN_NET_DEBT()) + ).add(th.toBN(1)); // add 1 to avoid rounding issues + const zusdAmount = MIN_DEBT.add(extraZUSDAmount); + + const price = await contracts.priceFeedTestnet.getPrice(); + const totalDebt = await th.getOpenTroveTotalDebt(contracts, zusdAmount); + const ICR = toBN(dec(2, 18)); + const borrowerOperationsCrossReentrancy = await BorrowerOperationsCrossReentrancy.new(contracts.borrowerOperations.address); + + await assertRevert( + borrowerOperationsCrossReentrancy.testCrossReentrancyAffectingDebt( + th._100pct, + extraZUSDAmount, + whale, + whale, + priceFeed.address, + {value: ICR.mul(totalDebt).div(price).mul(toBN(2))}), + "Recovery mode mutex locked. Try in another block" + ); + }); + + it("open trove, increase the debt, then decrease the debt in the same block should revert due to reentrancy", async () => { + await openTrove({ ICR: toBN(dec(2, 18)), extraParams: { from: alice } }); + await priceFeed.setPrice("105000000000000000000"); + assert.isTrue(await th.checkRecoveryMode(contracts)); + + const extraZUSDAmount = toBN(dec(1000, 18)); + const MIN_DEBT = ( + await th.getNetBorrowingAmount(contracts, await contracts.borrowerOperations.MIN_NET_DEBT()) + ).add(th.toBN(1)); // add 1 to avoid rounding issues + const zusdAmount = MIN_DEBT.add(extraZUSDAmount); + + const price = await contracts.priceFeedTestnet.getPrice(); + const totalDebt = await th.getOpenTroveTotalDebt(contracts, zusdAmount); + const ICR = toBN(dec(2, 18)); + + const borrowerOperationsCrossReentrancy = await BorrowerOperationsCrossReentrancy.new(contracts.borrowerOperations.address); + // Fund the contract with ETH for gas + await hre.network.provider.send("hardhat_setBalance", [ + borrowerOperationsCrossReentrancy.address, + "0x56BC75E2D63100000", // 100 ETH in hex + ]); + // Impersonate the borrower test contract + await hre.network.provider.request({ + method: "hardhat_impersonateAccount", + params: [borrowerOperationsCrossReentrancy.address], + }); + + await openTrove({ maxFeePercentage: th._100pct, extraZUSDAmount, whale, whale, ICR, extraParams: { value: ICR.mul(totalDebt).div(price).mul(toBN(10)), from: borrowerOperationsCrossReentrancy.address } }); + await priceFeed.setPrice("10000000000000000000"); + + const ZUSDwithdrawal = 1; // withdraw 1 wei ZUSD + const collateralAmount = dec(20000, 'ether'); // Increase collateral + + await assertRevert(borrowerOperationsCrossReentrancy.testCrossReentrancyByIncreasingDebt( + th._100pct, + ZUSDwithdrawal, + borrowerOperationsCrossReentrancy.address, + borrowerOperationsCrossReentrancy.address, + priceFeed.address, + {from: whale, value: collateralAmount}), "Recovery mode mutex locked. Try in another block") + }); }); if (!withProxy) {