From c9079ce122a5c8353824c6c5f9b24369e8521e58 Mon Sep 17 00:00:00 2001 From: cwsnt Date: Wed, 11 Dec 2024 17:00:52 +0700 Subject: [PATCH 1/9] feat: introduce reentrancy zero --- contracts/BorrowerOperations.sol | 24 ++-- .../reentrancy/SharedReentrancyGuard.sol | 61 ++++++++++ .../reentrancy/ZeroProtocolMutex.sol | 33 ++++++ contracts/Interfaces/IZeroProtocolMutex.sol | 6 + .../BorrowerOperationsCrossReentrancy.sol | 33 ++++++ .../TestNonReentrantValueSetter.sol | 24 ++++ contracts/TestContracts/TestValueSetter.sol | 14 +++ .../TestContracts/TestValueSetterProxy.sol | 9 ++ .../deploy/7-DeployZeroProtocolMutex.ts | 28 +++++ deployment/helpers/reentrancy/utils.js | 110 ++++++++++++++++++ tests/js/BorrowerOperationsTest.js | 26 +++++ tests/js/SharedReentrancyGuardTest.js | 98 ++++++++++++++++ 12 files changed, 455 insertions(+), 11 deletions(-) create mode 100644 contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol create mode 100644 contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol create mode 100644 contracts/Interfaces/IZeroProtocolMutex.sol create mode 100644 contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol create mode 100644 contracts/TestContracts/TestNonReentrantValueSetter.sol create mode 100644 contracts/TestContracts/TestValueSetter.sol create mode 100644 contracts/TestContracts/TestValueSetterProxy.sol create mode 100644 deployment/deploy/7-DeployZeroProtocolMutex.ts create mode 100644 deployment/helpers/reentrancy/utils.js create mode 100644 tests/js/SharedReentrancyGuardTest.js diff --git a/contracts/BorrowerOperations.sol b/contracts/BorrowerOperations.sol index 8043271..791ab96 100644 --- a/contracts/BorrowerOperations.sol +++ b/contracts/BorrowerOperations.sol @@ -16,12 +16,14 @@ import "./Dependencies/console.sol"; import "./BorrowerOperationsStorage.sol"; import "./Dependencies/Mynt/MyntLib.sol"; import "./Interfaces/IPermit2.sol"; +import "./Dependencies/reentrancy/SharedReentrancyGuard.sol"; contract BorrowerOperations is LiquityBase, BorrowerOperationsStorage, CheckContract, - IBorrowerOperations + IBorrowerOperations, + SharedReentrancyGuard { /** CONSTANT / IMMUTABLE VARIABLE ONLY */ IPermit2 public immutable permit2; @@ -169,7 +171,7 @@ contract BorrowerOperations is uint256 _ZUSDAmount, address _upperHint, address _lowerHint - ) external payable override { + ) external payable override nonReentrantAtOpening { _openTrove(_maxFeePercentage, _ZUSDAmount, _upperHint, _lowerHint, msg.sender); } @@ -178,7 +180,7 @@ contract BorrowerOperations is uint256 _ZUSDAmount, address _upperHint, address _lowerHint - ) external payable override { + ) external payable override nonReentrantAtOpening { require(address(massetManager) != address(0), "Masset address not set"); _openTrove(_maxFeePercentage, _ZUSDAmount, _upperHint, _lowerHint, address(this)); @@ -282,7 +284,7 @@ contract BorrowerOperations is } /// Send ETH as collateral to a trove - function addColl(address _upperHint, address _lowerHint) external payable override { + function addColl(address _upperHint, address _lowerHint) external payable override nonReentrantAtTroveAdjustment(false) { _adjustTrove(msg.sender, 0, 0, false, _upperHint, _lowerHint, 0); } @@ -291,7 +293,7 @@ contract BorrowerOperations is address _borrower, address _upperHint, address _lowerHint - ) external payable override { + ) external payable override nonReentrantAtTroveAdjustment(false) { _requireCallerIsStabilityPool(); _adjustTrove(_borrower, 0, 0, false, _upperHint, _lowerHint, 0); } @@ -301,7 +303,7 @@ contract BorrowerOperations is uint256 _collWithdrawal, address _upperHint, address _lowerHint - ) external override { + ) external override nonReentrantAtTroveAdjustment(false) { _adjustTrove(msg.sender, _collWithdrawal, 0, false, _upperHint, _lowerHint, 0); } @@ -311,7 +313,7 @@ contract BorrowerOperations is uint256 _ZUSDAmount, address _upperHint, address _lowerHint - ) external override { + ) external override nonReentrantAtTroveAdjustment(true) { _adjustTrove(msg.sender, 0, _ZUSDAmount, true, _upperHint, _lowerHint, _maxFeePercentage); } @@ -384,7 +386,7 @@ contract BorrowerOperations is bool _isDebtIncrease, address _upperHint, address _lowerHint - ) external payable override { + ) external payable override nonReentrantAtTroveAdjustment(_isDebtIncrease) { _adjustTrove( msg.sender, _collWithdrawal, @@ -692,11 +694,11 @@ contract BorrowerOperations is ); } - function closeTrove() external override { + function closeTrove() external override nonReentrantAtClosing { _closeTrove(); } - function closeNueTrove(IMassetManager.PermitParams calldata _permitParams) external override { + function closeNueTrove(IMassetManager.PermitParams calldata _permitParams) external override nonReentrantAtClosing { require(address(massetManager) != address(0), "Masset address not set"); uint256 debt = troveManager.getTroveDebt(msg.sender); @@ -710,7 +712,7 @@ contract BorrowerOperations is _closeTrove(); } - function closeNueTroveWithPermit2(ISignatureTransfer.PermitTransferFrom memory _permit, bytes calldata _signature) external override { + function closeNueTroveWithPermit2(ISignatureTransfer.PermitTransferFrom memory _permit, bytes calldata _signature) external override nonReentrantAtClosing { require(address(massetManager) != address(0), "Masset address not set"); uint256 debt = troveManager.getTroveDebt(msg.sender); diff --git a/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol b/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol new file mode 100644 index 0000000..3aaa493 --- /dev/null +++ b/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.6.11; + +import "../../Interfaces/IZeroProtocolMutex.sol"; + +/* + * @title contract for shared reentrancy guards + * + * @dev This contract exposes the modifiers for opening, closing, increasing, decreasing trove functionality + * The objective is we will not allow the opening, increase/decrease, closing functionalityto be executed in the same block + * + * @dev The ZeroProtocolMutex contract address is hardcoded because the address is deployed using a + * special deployment method (similar to ERC1820Registry). This contract therefore has no + * state and is thus safe to add to the inheritance chain of upgradeable contracts. + */ +contract SharedReentrancyGuard { + /* + * This is the address of the zero protocol mutex contract that will be used as the + * reentrancy guard. + * + * The address is hardcoded to avoid changing the memory layout of + * derived contracts (possibly upgradable). Hardcoding the address is possible, + * because the Mutex contract is always deployed to the same address, with the + * same method used in the deployment of ERC1820Registry. + */ + IZeroProtocolMutex private constant MUTEX = IZeroProtocolMutex(0x42B023F998d7B9c127e9bDcDCE57ccd1f5e1d919); + + /* + * This is the modifier that will be used to set the user's block number when opening/increasing trove + */ + modifier nonReentrantAtOpening() { + MUTEX.handleMutex(true); + + _; + } + + /* + * This is the modifier that will be used to check the user's block number to be not the same block + * And if the check pass, it will reset the user's block number to 0 + * when closing trove + */ + modifier nonReentrantAtClosing() { + MUTEX.handleMutex(false); + + _; + } + + /* + * In case of _isDebtIncrease true it will do the same behaviour as nonReentrantAtOpening + * Otherwise it will do the nonReentrantAtClosing + */ + modifier nonReentrantAtTroveAdjustment(bool _isDebtIncrease) { + if(_isDebtIncrease) { + MUTEX.handleMutex(true); + } else { + MUTEX.handleMutex(false); + } + + _; + } +} diff --git a/contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol b/contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol new file mode 100644 index 0000000..1ed7686 --- /dev/null +++ b/contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.6.11; + +/* + * @title Zero Protocol Mutex contract + * + * @notice A mutex mechanism contract that will handle some function executions not to be in the same block + */ +contract ZeroProtocolMutex { + /* + * We use an uint to store the mutex state. + */ + mapping(address => uint256) public userBlockNumber; + + /* + * @notice set the user's block number for opening, and do check & reset to 0 for closing + * + * @dev This is the function will be called by the open, close, increase, decrease trove function + */ + function handleMutex(bool _isOpening) external { + if(_isOpening) { + userBlockNumber[tx.origin] = block.number; + } else { + if(userBlockNumber[tx.origin] > 0) { + if(userBlockNumber[tx.origin] == block.number) { + revert("ZeroProtocolMutex: mutex locked"); + } + + userBlockNumber[tx.origin] = 0; + } + } + } +} diff --git a/contracts/Interfaces/IZeroProtocolMutex.sol b/contracts/Interfaces/IZeroProtocolMutex.sol new file mode 100644 index 0000000..784ab2a --- /dev/null +++ b/contracts/Interfaces/IZeroProtocolMutex.sol @@ -0,0 +1,6 @@ +pragma solidity 0.6.11; + +interface IZeroProtocolMutex { + function handleMutex(bool) external; + function userBlockNumber(address) external view returns(uint256); +} diff --git a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol new file mode 100644 index 0000000..95ba889 --- /dev/null +++ b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.6.11; + +import "../Interfaces/IBorrowerOperations.sol"; + +contract BorrowerOperationsCrossReentrancy { + IBorrowerOperations public borrowerOperations; + + constructor( + IBorrowerOperations _borrowerOperations + ) public { + borrowerOperations = _borrowerOperations; + } + + fallback() external payable {} + + function testCrossReentrancy( + uint256 _maxFeePercentage, + uint256 _ZUSDAmount, + address _upperHint, + address _lowerHint + ) public payable { + borrowerOperations.openTrove{value: msg.value}( + _maxFeePercentage, + _ZUSDAmount, + _upperHint, + _lowerHint + ); + + // // should revert due to reentrancy violation + borrowerOperations.closeTrove(); + } +} diff --git a/contracts/TestContracts/TestNonReentrantValueSetter.sol b/contracts/TestContracts/TestNonReentrantValueSetter.sol new file mode 100644 index 0000000..a23d882 --- /dev/null +++ b/contracts/TestContracts/TestNonReentrantValueSetter.sol @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.6.11; + +import "../Dependencies/reentrancy/SharedReentrancyGuard.sol"; + +contract TestNonReentrantValueSetter is SharedReentrancyGuard { + uint256 public value; + + function setValueOpening(uint256 newValue) public nonReentrantAtOpening { + value = newValue; + } + + function setValueClosing(uint256 newValue) public nonReentrantAtClosing { + value = newValue; + } + + // this will always fail + function setOtherContractValueNonReentrant( + address other, + uint256 newValue + ) external nonReentrantAtOpening { + TestNonReentrantValueSetter(other).setValueClosing(newValue); + } +} diff --git a/contracts/TestContracts/TestValueSetter.sol b/contracts/TestContracts/TestValueSetter.sol new file mode 100644 index 0000000..d06bd84 --- /dev/null +++ b/contracts/TestContracts/TestValueSetter.sol @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.6.11; + +contract TestValueSetter { + uint256 public value; + + function setValueOpening(uint256 newValue) public { + value = newValue; + } + + function setValueClosing(uint256 newValue) public { + value = newValue; + } +} diff --git a/contracts/TestContracts/TestValueSetterProxy.sol b/contracts/TestContracts/TestValueSetterProxy.sol new file mode 100644 index 0000000..1610959 --- /dev/null +++ b/contracts/TestContracts/TestValueSetterProxy.sol @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.6.11; + +import "../Proxy/UpgradableProxy.sol"; + +contract TestValueSetterProxy is UpgradableProxy { + // This is here for the memory layout + uint256 public value; +} diff --git a/deployment/deploy/7-DeployZeroProtocolMutex.ts b/deployment/deploy/7-DeployZeroProtocolMutex.ts new file mode 100644 index 0000000..29fa827 --- /dev/null +++ b/deployment/deploy/7-DeployZeroProtocolMutex.ts @@ -0,0 +1,28 @@ +const Logs = require("node-logs"); +const logger = new Logs().showInConsole(true); +const { SAVED_DEPLOY_DATA_ZERO, getOrDeployZeroProtocolMutex, createZeroProtocolMutexDeployTransaction } = require("../helpers/reentrancy/utils"); + +const func = async function (hre) { + const { + deployments: { deploy, log, getOrNull }, + getNamedAccounts, + network, + ethers, + } = hre; + const { deployerAddress, contractAddress } = SAVED_DEPLOY_DATA_ZERO; + logger.warn("Deploying Zero Protocol Mutex..."); + + if (ethers.provider.getBalance(deployerAddress) === 0) { + throw new Error("Deployer balance is zero"); + } + + console.log(await createZeroProtocolMutexDeployTransaction()); + + const zeroProtocolMutex = await getOrDeployZeroProtocolMutex(); + if (zeroProtocolMutex.target !== contractAddress) { + throw new Error(`Mutex address is ${zeroProtocolMutex.target}, expected ${contractAddress}`); + } + logger.warn("Zero Protocol Mutex deployed"); +}; +func.tags = ["ZeroProtocolMutex"]; +module.exports = func; diff --git a/deployment/helpers/reentrancy/utils.js b/deployment/helpers/reentrancy/utils.js new file mode 100644 index 0000000..83c804d --- /dev/null +++ b/deployment/helpers/reentrancy/utils.js @@ -0,0 +1,110 @@ +const { ethers } = require("hardhat"); +const testHelpers = require("../../../utils/js/testHelpers.js"); +const { Transaction, Wallet, Provider, getCreateAddress } = require("ethers"); + +const th = testHelpers.TestHelper; +const toBN = th.toBN; + +const SAVED_DEPLOY_DATA_ZERO = { + serializedDeployTx: + "0xf901f8808403ef14808302194d8080b901a6608060405234801561001057600080fd5b50610186806100206000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c80636981665b1461003b57806369b13ba014610073575b600080fd5b6100616004803603602081101561005157600080fd5b50356001600160a01b0316610094565b60408051918252519081900360200190f35b6100926004803603602081101561008957600080fd5b503515156100a6565b005b60006020819052908152604090205481565b80156100c35732600090815260208190526040902043905561014d565b326000908152602081905260409020541561014d573260009081526020819052604090205443141561013c576040805162461bcd60e51b815260206004820152601f60248201527f5a65726f50726f746f636f6c4d757465783a206d75746578206c6f636b656400604482015290519081900360640190fd5b326000908152602081905260408120555b5056fea26469706673582212208fa44190280703d199dee337f61f207770f627ddf810bdbd4a1c74fcb8b31e2664736f6c634300060b00331ba06d757465786d757465786d757465786d757465786d757465786d757465786d75a06d757465786d757465786d757465786d757465786d757465786d757465786d75", + deployerAddress: "0xb339D675F5Fb2EEec8a13db76dD57C11F4Af3869", + contractAddress: "0x42B023F998d7B9c127e9bDcDCE57ccd1f5e1d919", + transactionCostWei: toBN(9078234000000), +}; + +const getOrDeployZeroProtocolMutex = async () => { + const provider = ethers.provider; + + const { serializedDeployTx, deployerAddress, contractAddress, transactionCostWei } = + SAVED_DEPLOY_DATA_ZERO; + const ZeroProtocolMutex = await ethers.getContractAt("ZeroProtocolMutex", contractAddress); + const deployedCode = await provider.getCode(contractAddress); + if (deployedCode.replace(/0+$/) !== "0x") { + // Contract is deployed + // it's practically impossible to deploy to this address with malicious bytecode so we don't need to check + return ZeroProtocolMutex; + } + + // Not deployed, we need to deploy + console.log("ZeroProtocolMutex has not been deployed, deploying...") + + // Fund the account + const deployerBalance = await provider.getBalance(deployerAddress); + const whale = (await ethers.getSigners())[0]; + if (deployerBalance < transactionCostWei) { + const requiredBalance = toBN(transactionCostWei.toString()).sub(toBN(deployerBalance.toString())); + const tx = await whale.sendTransaction({ + to: deployerAddress, + value: requiredBalance.toString(), + }); + await tx.wait(); + } + + const tx = await provider.broadcastTransaction(serializedDeployTx); + await tx.wait(); + + return ZeroProtocolMutex.attach(contractAddress); +}; + +async function createZeroProtocolMutexDeployTransaction() { + const provider = ethers.provider; + const ZeroProtocolMutex = await ethers.getContractFactory("ZeroProtocolMutex"); + const { data: bytecode } = await ZeroProtocolMutex.getDeployTransaction(); + console.log(bytecode) + + const signature = { + v: 27, // must not be eip-155 to allow cross-chain deployments + // "mutex" in hex: 6d75746578 + // 0xm u t e x m u t e x m u t e x m u t e x m u t e x m u t e x m u + r: "0x6d757465786d757465786d757465786d757465786d757465786d757465786d75", + s: "0x6d757465786d757465786d757465786d757465786d757465786d757465786d75", + }; + + const hardhatGasLimit = await provider.estimateGas({ data: bytecode }); + const gasLimit = toBN(137549); + if (hardhatGasLimit > gasLimit) { + throw new Error( + `Hardhat estimates the gas limit as ${hardhatGasLimit.toString()}, ` + + `which is higher than the hardcoded gas limit ${gasLimit.toString()}` + ); + } + + // 10 gwei, should be enough to also mine on other chains. Could also be 100 like with erc1820 + const gasPrice = toBN(66000000); + + const transactionCostWei = gasLimit.mul(gasPrice); + + const deployTx = { + data: bytecode, // We could hardcode this too + nonce: 0, + gasLimit: gasLimit.toString(), + gasPrice: gasPrice.toString(), + type: 0 + }; + + const transaction = Transaction.from({...deployTx, signature}); + const serializedDeployTx = transaction.serialized; + + console.log(transactionCostWei.toString()) + + const parsedDeployTx = Transaction.from(serializedDeployTx); + const contractAddress = await getCreateAddress({ + from: parsedDeployTx.from, + nonce: parsedDeployTx.nonce, + }); + const deployerAddress = parsedDeployTx.from; + + return { + serializedDeployTx, + deployerAddress, + contractAddress, + transactionCostWei, + }; +} + +module.exports = { + getOrDeployZeroProtocolMutex, + createZeroProtocolMutexDeployTransaction, + SAVED_DEPLOY_DATA_ZERO +}; diff --git a/tests/js/BorrowerOperationsTest.js b/tests/js/BorrowerOperationsTest.js index 0fbb096..f2c5ce4 100644 --- a/tests/js/BorrowerOperationsTest.js +++ b/tests/js/BorrowerOperationsTest.js @@ -10,7 +10,9 @@ 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 { getOrDeployZeroProtocolMutex } = require("../../deployment/helpers/reentrancy/utils"); const th = testHelpers.TestHelper; @@ -184,6 +186,9 @@ contract("BorrowerOperations", async accounts => { let revertToSnapshot; beforeEach(async () => { + // The Mutex singleton must be deployed for SharedReentrancyGuard to work + await getOrDeployZeroProtocolMutex(); + let snapshot = await timeMachine.takeSnapshot(); revertToSnapshot = () => timeMachine.revertToSnapshot(snapshot["result"]); }); @@ -6995,6 +7000,27 @@ contract("BorrowerOperations", async accounts => { assert.isTrue(newTCR.eq(expectedTCR)); }); + + it("openTrove(): open a Trove, then close it in the same block should revert", async () => { + + const extraZUSDAmount = toBN(dec(10000, 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 th.assertRevert(borrowerOperationsCrossReentrancy.testCrossReentrancy( + th._100pct, + extraZUSDAmount, + whale, + whale, + {value: ICR.mul(totalDebt).div(price)} + ), "ZeroProtocolMutex: mutex locked"); + }); }); if (!withProxy) { diff --git a/tests/js/SharedReentrancyGuardTest.js b/tests/js/SharedReentrancyGuardTest.js new file mode 100644 index 0000000..1f6c84a --- /dev/null +++ b/tests/js/SharedReentrancyGuardTest.js @@ -0,0 +1,98 @@ +const { ethers } = require("hardhat"); +const { getOrDeployZeroProtocolMutex } = require("../../deployment/helpers/reentrancy/utils"); + +describe("SharedReentrancyGuard", async () => { + let nonReentrantValueSetter; + let anotherNonReentrantValueSetter; + let reentrantValueSetter; + let valueSetterProxy; + let proxiedValueSetter; + + beforeEach(async () => { + const NonReentrantValueSetter = await ethers.getContractFactory( + "TestNonReentrantValueSetter" + ); + + const ValueSetter = await ethers.getContractFactory("TestValueSetter"); + const ValueSetterProxy = await ethers.getContractFactory("TestValueSetterProxy"); + nonReentrantValueSetter = await NonReentrantValueSetter.deploy(); + anotherNonReentrantValueSetter = await NonReentrantValueSetter.deploy(); + reentrantValueSetter = await ValueSetter.deploy(); + valueSetterProxy = await ValueSetterProxy.deploy(); + proxiedValueSetter = await ValueSetter.attach(valueSetterProxy.target); + + // The Mutex singleton must be deployed for SharedReentrancyGuard to work + await getOrDeployZeroProtocolMutex(); + }); + + it("sanity check", async () => { + expect(await nonReentrantValueSetter.value()).to.equal(0); + expect(await anotherNonReentrantValueSetter.value()).to.equal(0); + expect(await reentrantValueSetter.value()).to.equal(0); + expect(await valueSetterProxy.value()).to.equal(0); + expect(await proxiedValueSetter.value()).to.equal(0); + + await nonReentrantValueSetter.setValueOpening(1); + await anotherNonReentrantValueSetter.setValueOpening(2); + await reentrantValueSetter.setValueOpening(3); + + await valueSetterProxy.setImplementation(reentrantValueSetter.target); + await proxiedValueSetter.setValueOpening(4); + + expect(await nonReentrantValueSetter.value()).to.equal(1); + expect(await anotherNonReentrantValueSetter.value()).to.equal(2); + expect(await reentrantValueSetter.value()).to.equal(3); + expect(await valueSetterProxy.value()).to.equal(4); + expect(await proxiedValueSetter.value()).to.equal(4); + }); + + it("globallyNonReentrant call from globallyNonReentrant call reverts", async () => { + await expect( + nonReentrantValueSetter.setOtherContractValueNonReentrant( + anotherNonReentrantValueSetter.target, + 1 + ) + ).to.be.revertedWith("ZeroProtocolMutex: mutex locked"); + expect(await anotherNonReentrantValueSetter.value()).to.equal(0); + }); + + it("non-globallyNonReentrant call from globallyNonReentrant call does not revert", async () => { + await nonReentrantValueSetter.setOtherContractValueNonReentrant( + reentrantValueSetter.target, + 1 + ); + expect(await reentrantValueSetter.value()).to.equal(1); + }); + + it("globallyNonReentrant works with proxies", async () => { + await valueSetterProxy.setImplementation(reentrantValueSetter.target); + expect(await proxiedValueSetter.value()).to.equal(0); + + await nonReentrantValueSetter.setOtherContractValueNonReentrant( + valueSetterProxy.target, + 1 + ); + expect(await proxiedValueSetter.value()).to.equal(1); + + await valueSetterProxy.setImplementation(anotherNonReentrantValueSetter.target); + await expect( + nonReentrantValueSetter.setOtherContractValueNonReentrant(valueSetterProxy.target, 2) + ).to.be.revertedWith("ZeroProtocolMutex: mutex locked"); + expect(await proxiedValueSetter.value()).to.equal(1); + + await proxiedValueSetter.setValueOpening(3); + expect(await proxiedValueSetter.value()).to.equal(3); + }); + + it("works with proxies without breaking the memory layout", async () => { + await valueSetterProxy.setImplementation(reentrantValueSetter.target); + expect(await proxiedValueSetter.value()).to.equal(0); + + await proxiedValueSetter.setValueOpening(1); + expect(await proxiedValueSetter.value()).to.equal(1); + + await valueSetterProxy.setImplementation(anotherNonReentrantValueSetter.target); + await proxiedValueSetter.setValueOpening(2); + expect(await proxiedValueSetter.value()).to.equal(2); + }); +}); From be677f9c5545c893edbb356f361b906e85cbbdaa Mon Sep 17 00:00:00 2001 From: cwsnt Date: Wed, 11 Dec 2024 17:32:43 +0700 Subject: [PATCH 2/9] fix: test case --- tests/js/AccessControlTest.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/js/AccessControlTest.js b/tests/js/AccessControlTest.js index b59055f..f5d9e74 100644 --- a/tests/js/AccessControlTest.js +++ b/tests/js/AccessControlTest.js @@ -2,6 +2,7 @@ const deploymentHelper = require("../../utils/js/deploymentHelpers.js"); const testHelpers = require("../../utils/js/testHelpers.js"); const TroveManagerTester = artifacts.require("TroveManagerTester"); const PriceFeedSovryn = artifacts.require("PriceFeedSovrynTester"); +const { getOrDeployZeroProtocolMutex } = require("../../deployment/helpers/reentrancy/utils"); const th = testHelpers.TestHelper; const timeValues = testHelpers.TimeValues; @@ -39,6 +40,8 @@ contract( let communityIssuance; before(async () => { + await getOrDeployZeroProtocolMutex(); + coreContracts = await deploymentHelper.deployLiquityCore(); coreContracts.troveManager = await TroveManagerTester.new(coreContracts.permit2.address); coreContracts = await deploymentHelper.deployZUSDTokenTester(coreContracts); From 55c2b840522e062f36ceb1cd181b3a2551bc0d4a Mon Sep 17 00:00:00 2001 From: cwsnt Date: Thu, 12 Dec 2024 14:13:03 +0700 Subject: [PATCH 3/9] consider the recovery mode only for the reentrancy --- contracts/BorrowerOperations.sol | 26 ++++++++----- .../reentrancy/SharedReentrancyGuard.sol | 37 ++++--------------- .../BorrowerOperationsCrossReentrancy.sol | 12 +++++- .../TestNonReentrantValueSetter.sol | 9 +++-- tests/js/BorrowerOperationsTest.js | 26 ++++++++----- 5 files changed, 56 insertions(+), 54 deletions(-) diff --git a/contracts/BorrowerOperations.sol b/contracts/BorrowerOperations.sol index 791ab96..304824d 100644 --- a/contracts/BorrowerOperations.sol +++ b/contracts/BorrowerOperations.sol @@ -171,7 +171,7 @@ contract BorrowerOperations is uint256 _ZUSDAmount, address _upperHint, address _lowerHint - ) external payable override nonReentrantAtOpening { + ) external payable override { _openTrove(_maxFeePercentage, _ZUSDAmount, _upperHint, _lowerHint, msg.sender); } @@ -180,7 +180,7 @@ contract BorrowerOperations is uint256 _ZUSDAmount, address _upperHint, address _lowerHint - ) external payable override nonReentrantAtOpening { + ) external payable override { require(address(massetManager) != address(0), "Masset address not set"); _openTrove(_maxFeePercentage, _ZUSDAmount, _upperHint, _lowerHint, address(this)); @@ -205,6 +205,8 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); bool isRecoveryMode = _checkRecoveryMode(vars.price); + if(isRecoveryMode) nonReentrantCheck(true); + _requireValidMaxFeePercentage(_maxFeePercentage, isRecoveryMode); _requireTroveisNotActive(contractsCache.troveManager, msg.sender); @@ -284,7 +286,7 @@ contract BorrowerOperations is } /// Send ETH as collateral to a trove - function addColl(address _upperHint, address _lowerHint) external payable override nonReentrantAtTroveAdjustment(false) { + function addColl(address _upperHint, address _lowerHint) external payable override { _adjustTrove(msg.sender, 0, 0, false, _upperHint, _lowerHint, 0); } @@ -293,7 +295,7 @@ contract BorrowerOperations is address _borrower, address _upperHint, address _lowerHint - ) external payable override nonReentrantAtTroveAdjustment(false) { + ) external payable override { _requireCallerIsStabilityPool(); _adjustTrove(_borrower, 0, 0, false, _upperHint, _lowerHint, 0); } @@ -303,7 +305,7 @@ contract BorrowerOperations is uint256 _collWithdrawal, address _upperHint, address _lowerHint - ) external override nonReentrantAtTroveAdjustment(false) { + ) external override { _adjustTrove(msg.sender, _collWithdrawal, 0, false, _upperHint, _lowerHint, 0); } @@ -313,7 +315,7 @@ contract BorrowerOperations is uint256 _ZUSDAmount, address _upperHint, address _lowerHint - ) external override nonReentrantAtTroveAdjustment(true) { + ) external override { _adjustTrove(msg.sender, 0, _ZUSDAmount, true, _upperHint, _lowerHint, _maxFeePercentage); } @@ -386,7 +388,7 @@ contract BorrowerOperations is bool _isDebtIncrease, address _upperHint, address _lowerHint - ) external payable override nonReentrantAtTroveAdjustment(_isDebtIncrease) { + ) external payable override { _adjustTrove( msg.sender, _collWithdrawal, @@ -587,6 +589,10 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); vars.isRecoveryMode = _checkRecoveryMode(vars.price); + if(vars.isRecoveryMode) { + nonReentrantCheck(_isDebtIncrease); + } + if (_isDebtIncrease) { _requireValidMaxFeePercentage(_maxFeePercentage, vars.isRecoveryMode); _requireNonZeroDebtChange(_ZUSDChange); @@ -694,11 +700,11 @@ contract BorrowerOperations is ); } - function closeTrove() external override nonReentrantAtClosing { + function closeTrove() external override { _closeTrove(); } - function closeNueTrove(IMassetManager.PermitParams calldata _permitParams) external override nonReentrantAtClosing { + function closeNueTrove(IMassetManager.PermitParams calldata _permitParams) external override { require(address(massetManager) != address(0), "Masset address not set"); uint256 debt = troveManager.getTroveDebt(msg.sender); @@ -712,7 +718,7 @@ contract BorrowerOperations is _closeTrove(); } - function closeNueTroveWithPermit2(ISignatureTransfer.PermitTransferFrom memory _permit, bytes calldata _signature) external override nonReentrantAtClosing { + function closeNueTroveWithPermit2(ISignatureTransfer.PermitTransferFrom memory _permit, bytes calldata _signature) external override { require(address(massetManager) != address(0), "Masset address not set"); uint256 debt = troveManager.getTroveDebt(msg.sender); diff --git a/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol b/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol index 3aaa493..af178e1 100644 --- a/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol +++ b/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol @@ -26,36 +26,13 @@ contract SharedReentrancyGuard { IZeroProtocolMutex private constant MUTEX = IZeroProtocolMutex(0x42B023F998d7B9c127e9bDcDCE57ccd1f5e1d919); /* - * This is the modifier that will be used to set the user's block number when opening/increasing trove + * @dev function that is responsible to handle the mutex (user's block number) check + * @param _isOpening flag whether it is true (opening, increasing), and false (closing, decreasing) + * If it is true, it will just set the user's block number to the current block number + * If it is false and the user's block number was not 0, it will check the user's block number to be not the same block + * and if the check pass, it will reset the user's block number to 0 */ - modifier nonReentrantAtOpening() { - MUTEX.handleMutex(true); - - _; - } - - /* - * This is the modifier that will be used to check the user's block number to be not the same block - * And if the check pass, it will reset the user's block number to 0 - * when closing trove - */ - modifier nonReentrantAtClosing() { - MUTEX.handleMutex(false); - - _; - } - - /* - * In case of _isDebtIncrease true it will do the same behaviour as nonReentrantAtOpening - * Otherwise it will do the nonReentrantAtClosing - */ - modifier nonReentrantAtTroveAdjustment(bool _isDebtIncrease) { - if(_isDebtIncrease) { - MUTEX.handleMutex(true); - } else { - MUTEX.handleMutex(false); - } - - _; + function nonReentrantCheck(bool _isOpening) internal { + MUTEX.handleMutex(_isOpening); } } diff --git a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol index 95ba889..f3452d6 100644 --- a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol +++ b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol @@ -3,6 +3,10 @@ pragma solidity 0.6.11; import "../Interfaces/IBorrowerOperations.sol"; +interface IPriceFeedTestnet { + function setPrice(uint256 price) external returns (bool); +} + contract BorrowerOperationsCrossReentrancy { IBorrowerOperations public borrowerOperations; @@ -18,7 +22,8 @@ contract BorrowerOperationsCrossReentrancy { uint256 _maxFeePercentage, uint256 _ZUSDAmount, address _upperHint, - address _lowerHint + address _lowerHint, + address _priceFeed ) public payable { borrowerOperations.openTrove{value: msg.value}( _maxFeePercentage, @@ -27,7 +32,10 @@ contract BorrowerOperationsCrossReentrancy { _lowerHint ); + // manipulate the price so that the recovery mode will be triggered + IPriceFeedTestnet(_priceFeed).setPrice(1e8); + // // should revert due to reentrancy violation - borrowerOperations.closeTrove(); + borrowerOperations.addColl(_upperHint, _lowerHint); } } diff --git a/contracts/TestContracts/TestNonReentrantValueSetter.sol b/contracts/TestContracts/TestNonReentrantValueSetter.sol index a23d882..2fff9d5 100644 --- a/contracts/TestContracts/TestNonReentrantValueSetter.sol +++ b/contracts/TestContracts/TestNonReentrantValueSetter.sol @@ -6,11 +6,13 @@ import "../Dependencies/reentrancy/SharedReentrancyGuard.sol"; contract TestNonReentrantValueSetter is SharedReentrancyGuard { uint256 public value; - function setValueOpening(uint256 newValue) public nonReentrantAtOpening { + function setValueOpening(uint256 newValue) public { + nonReentrantCheck(true); value = newValue; } - function setValueClosing(uint256 newValue) public nonReentrantAtClosing { + function setValueClosing(uint256 newValue) public { + nonReentrantCheck(false); value = newValue; } @@ -18,7 +20,8 @@ contract TestNonReentrantValueSetter is SharedReentrancyGuard { function setOtherContractValueNonReentrant( address other, uint256 newValue - ) external nonReentrantAtOpening { + ) external { + nonReentrantCheck(true); TestNonReentrantValueSetter(other).setValueClosing(newValue); } } diff --git a/tests/js/BorrowerOperationsTest.js b/tests/js/BorrowerOperationsTest.js index f2c5ce4..237bc78 100644 --- a/tests/js/BorrowerOperationsTest.js +++ b/tests/js/BorrowerOperationsTest.js @@ -7001,9 +7001,13 @@ contract("BorrowerOperations", async accounts => { assert.isTrue(newTCR.eq(expectedTCR)); }); - it("openTrove(): open a Trove, then close it in the same block should revert", async () => { + it("openTrove(): open a Trove, then adjust it in the same block should revert", async () => { + await openTrove({ ICR: toBN(dec(2, 18)), extraParams: { from: alice } }); + await priceFeed.setPrice("105000000000000000000"); - const extraZUSDAmount = toBN(dec(10000, 18)); + 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 @@ -7013,13 +7017,17 @@ contract("BorrowerOperations", async accounts => { const totalDebt = await th.getOpenTroveTotalDebt(contracts, zusdAmount); const ICR = toBN(dec(2, 18)); const borrowerOperationsCrossReentrancy = await BorrowerOperationsCrossReentrancy.new(contracts.borrowerOperations.address); - await th.assertRevert(borrowerOperationsCrossReentrancy.testCrossReentrancy( - th._100pct, - extraZUSDAmount, - whale, - whale, - {value: ICR.mul(totalDebt).div(price)} - ), "ZeroProtocolMutex: mutex locked"); + + await assertRevert( + borrowerOperationsCrossReentrancy.testCrossReentrancy( + th._100pct, + extraZUSDAmount, + whale, + whale, + priceFeed.address, + {value: ICR.mul(totalDebt).div(price)}), + "ZeroProtocolMutex: mutex locked" + ); }); }); From 8a3e4f52d8e5ecf3f7bc2286cc54316c0ffd815f Mon Sep 17 00:00:00 2001 From: cwsnt Date: Tue, 17 Dec 2024 22:29:02 +0700 Subject: [PATCH 4/9] move the mutex check to borrowerOperation only --- contracts/BorrowerOperations.sol | 28 ++++- contracts/BorrowerOperationsStorage.sol | 5 + .../reentrancy/SharedReentrancyGuard.sol | 38 ------ .../reentrancy/ZeroProtocolMutex.sol | 33 ------ contracts/Interfaces/IZeroProtocolMutex.sol | 6 - .../TestNonReentrantValueSetter.sol | 27 ----- contracts/TestContracts/TestValueSetter.sol | 14 --- .../TestContracts/TestValueSetterProxy.sol | 9 -- .../deploy/7-DeployZeroProtocolMutex.ts | 28 ----- deployment/helpers/reentrancy/utils.js | 110 ------------------ tests/js/AccessControlTest.js | 3 - tests/js/BorrowerOperationsTest.js | 4 - tests/js/SharedReentrancyGuardTest.js | 98 ---------------- 13 files changed, 27 insertions(+), 376 deletions(-) delete mode 100644 contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol delete mode 100644 contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol delete mode 100644 contracts/Interfaces/IZeroProtocolMutex.sol delete mode 100644 contracts/TestContracts/TestNonReentrantValueSetter.sol delete mode 100644 contracts/TestContracts/TestValueSetter.sol delete mode 100644 contracts/TestContracts/TestValueSetterProxy.sol delete mode 100644 deployment/deploy/7-DeployZeroProtocolMutex.ts delete mode 100644 deployment/helpers/reentrancy/utils.js delete mode 100644 tests/js/SharedReentrancyGuardTest.js diff --git a/contracts/BorrowerOperations.sol b/contracts/BorrowerOperations.sol index 304824d..129b193 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; @@ -16,14 +15,12 @@ import "./Dependencies/console.sol"; import "./BorrowerOperationsStorage.sol"; import "./Dependencies/Mynt/MyntLib.sol"; import "./Interfaces/IPermit2.sol"; -import "./Dependencies/reentrancy/SharedReentrancyGuard.sol"; contract BorrowerOperations is LiquityBase, BorrowerOperationsStorage, CheckContract, - IBorrowerOperations, - SharedReentrancyGuard + IBorrowerOperations { /** CONSTANT / IMMUTABLE VARIABLE ONLY */ IPermit2 public immutable permit2; @@ -161,6 +158,25 @@ contract BorrowerOperations is emit ZEROStakingAddressChanged(_zeroStakingAddress); } + /* + * @notice set the user's block number for opening, and do check & reset to 0 for closing + * + * @dev This is the function will be called by the open, close, increase, decrease trove function + */ + function notInTheSameBlockHandler(bool _isOpening) private { + if(_isOpening) { + userBlockNumber[tx.origin] = block.number; + } else { + if(userBlockNumber[tx.origin] > 0) { + if(userBlockNumber[tx.origin] == block.number) { + revert("ZeroProtocolMutex: mutex locked"); + } + + userBlockNumber[tx.origin] = 0; + } + } + } + function setMassetManagerAddress(address _massetManagerAddress) external onlyOwner { massetManager = IMassetManager(_massetManagerAddress); emit MassetManagerAddressChanged(_massetManagerAddress); @@ -205,7 +221,7 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); bool isRecoveryMode = _checkRecoveryMode(vars.price); - if(isRecoveryMode) nonReentrantCheck(true); + if(isRecoveryMode) notInTheSameBlockHandler(true); _requireValidMaxFeePercentage(_maxFeePercentage, isRecoveryMode); _requireTroveisNotActive(contractsCache.troveManager, msg.sender); @@ -590,7 +606,7 @@ contract BorrowerOperations is vars.isRecoveryMode = _checkRecoveryMode(vars.price); if(vars.isRecoveryMode) { - nonReentrantCheck(_isDebtIncrease); + notInTheSameBlockHandler(_isDebtIncrease); } if (_isDebtIncrease) { diff --git a/contracts/BorrowerOperationsStorage.sol b/contracts/BorrowerOperationsStorage.sol index 9e7d769..6eb7126 100644 --- a/contracts/BorrowerOperationsStorage.sol +++ b/contracts/BorrowerOperationsStorage.sol @@ -36,4 +36,9 @@ contract BorrowerOperationsStorage is Ownable { IMassetManager public massetManager; IFeeDistributor public feeDistributor; + + /* + * to store the user's block number + */ + mapping(address => uint256) public userBlockNumber; } diff --git a/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol b/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol deleted file mode 100644 index af178e1..0000000 --- a/contracts/Dependencies/reentrancy/SharedReentrancyGuard.sol +++ /dev/null @@ -1,38 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.6.11; - -import "../../Interfaces/IZeroProtocolMutex.sol"; - -/* - * @title contract for shared reentrancy guards - * - * @dev This contract exposes the modifiers for opening, closing, increasing, decreasing trove functionality - * The objective is we will not allow the opening, increase/decrease, closing functionalityto be executed in the same block - * - * @dev The ZeroProtocolMutex contract address is hardcoded because the address is deployed using a - * special deployment method (similar to ERC1820Registry). This contract therefore has no - * state and is thus safe to add to the inheritance chain of upgradeable contracts. - */ -contract SharedReentrancyGuard { - /* - * This is the address of the zero protocol mutex contract that will be used as the - * reentrancy guard. - * - * The address is hardcoded to avoid changing the memory layout of - * derived contracts (possibly upgradable). Hardcoding the address is possible, - * because the Mutex contract is always deployed to the same address, with the - * same method used in the deployment of ERC1820Registry. - */ - IZeroProtocolMutex private constant MUTEX = IZeroProtocolMutex(0x42B023F998d7B9c127e9bDcDCE57ccd1f5e1d919); - - /* - * @dev function that is responsible to handle the mutex (user's block number) check - * @param _isOpening flag whether it is true (opening, increasing), and false (closing, decreasing) - * If it is true, it will just set the user's block number to the current block number - * If it is false and the user's block number was not 0, it will check the user's block number to be not the same block - * and if the check pass, it will reset the user's block number to 0 - */ - function nonReentrantCheck(bool _isOpening) internal { - MUTEX.handleMutex(_isOpening); - } -} diff --git a/contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol b/contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol deleted file mode 100644 index 1ed7686..0000000 --- a/contracts/Dependencies/reentrancy/ZeroProtocolMutex.sol +++ /dev/null @@ -1,33 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.6.11; - -/* - * @title Zero Protocol Mutex contract - * - * @notice A mutex mechanism contract that will handle some function executions not to be in the same block - */ -contract ZeroProtocolMutex { - /* - * We use an uint to store the mutex state. - */ - mapping(address => uint256) public userBlockNumber; - - /* - * @notice set the user's block number for opening, and do check & reset to 0 for closing - * - * @dev This is the function will be called by the open, close, increase, decrease trove function - */ - function handleMutex(bool _isOpening) external { - if(_isOpening) { - userBlockNumber[tx.origin] = block.number; - } else { - if(userBlockNumber[tx.origin] > 0) { - if(userBlockNumber[tx.origin] == block.number) { - revert("ZeroProtocolMutex: mutex locked"); - } - - userBlockNumber[tx.origin] = 0; - } - } - } -} diff --git a/contracts/Interfaces/IZeroProtocolMutex.sol b/contracts/Interfaces/IZeroProtocolMutex.sol deleted file mode 100644 index 784ab2a..0000000 --- a/contracts/Interfaces/IZeroProtocolMutex.sol +++ /dev/null @@ -1,6 +0,0 @@ -pragma solidity 0.6.11; - -interface IZeroProtocolMutex { - function handleMutex(bool) external; - function userBlockNumber(address) external view returns(uint256); -} diff --git a/contracts/TestContracts/TestNonReentrantValueSetter.sol b/contracts/TestContracts/TestNonReentrantValueSetter.sol deleted file mode 100644 index 2fff9d5..0000000 --- a/contracts/TestContracts/TestNonReentrantValueSetter.sol +++ /dev/null @@ -1,27 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.6.11; - -import "../Dependencies/reentrancy/SharedReentrancyGuard.sol"; - -contract TestNonReentrantValueSetter is SharedReentrancyGuard { - uint256 public value; - - function setValueOpening(uint256 newValue) public { - nonReentrantCheck(true); - value = newValue; - } - - function setValueClosing(uint256 newValue) public { - nonReentrantCheck(false); - value = newValue; - } - - // this will always fail - function setOtherContractValueNonReentrant( - address other, - uint256 newValue - ) external { - nonReentrantCheck(true); - TestNonReentrantValueSetter(other).setValueClosing(newValue); - } -} diff --git a/contracts/TestContracts/TestValueSetter.sol b/contracts/TestContracts/TestValueSetter.sol deleted file mode 100644 index d06bd84..0000000 --- a/contracts/TestContracts/TestValueSetter.sol +++ /dev/null @@ -1,14 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.6.11; - -contract TestValueSetter { - uint256 public value; - - function setValueOpening(uint256 newValue) public { - value = newValue; - } - - function setValueClosing(uint256 newValue) public { - value = newValue; - } -} diff --git a/contracts/TestContracts/TestValueSetterProxy.sol b/contracts/TestContracts/TestValueSetterProxy.sol deleted file mode 100644 index 1610959..0000000 --- a/contracts/TestContracts/TestValueSetterProxy.sol +++ /dev/null @@ -1,9 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.6.11; - -import "../Proxy/UpgradableProxy.sol"; - -contract TestValueSetterProxy is UpgradableProxy { - // This is here for the memory layout - uint256 public value; -} diff --git a/deployment/deploy/7-DeployZeroProtocolMutex.ts b/deployment/deploy/7-DeployZeroProtocolMutex.ts deleted file mode 100644 index 29fa827..0000000 --- a/deployment/deploy/7-DeployZeroProtocolMutex.ts +++ /dev/null @@ -1,28 +0,0 @@ -const Logs = require("node-logs"); -const logger = new Logs().showInConsole(true); -const { SAVED_DEPLOY_DATA_ZERO, getOrDeployZeroProtocolMutex, createZeroProtocolMutexDeployTransaction } = require("../helpers/reentrancy/utils"); - -const func = async function (hre) { - const { - deployments: { deploy, log, getOrNull }, - getNamedAccounts, - network, - ethers, - } = hre; - const { deployerAddress, contractAddress } = SAVED_DEPLOY_DATA_ZERO; - logger.warn("Deploying Zero Protocol Mutex..."); - - if (ethers.provider.getBalance(deployerAddress) === 0) { - throw new Error("Deployer balance is zero"); - } - - console.log(await createZeroProtocolMutexDeployTransaction()); - - const zeroProtocolMutex = await getOrDeployZeroProtocolMutex(); - if (zeroProtocolMutex.target !== contractAddress) { - throw new Error(`Mutex address is ${zeroProtocolMutex.target}, expected ${contractAddress}`); - } - logger.warn("Zero Protocol Mutex deployed"); -}; -func.tags = ["ZeroProtocolMutex"]; -module.exports = func; diff --git a/deployment/helpers/reentrancy/utils.js b/deployment/helpers/reentrancy/utils.js deleted file mode 100644 index 83c804d..0000000 --- a/deployment/helpers/reentrancy/utils.js +++ /dev/null @@ -1,110 +0,0 @@ -const { ethers } = require("hardhat"); -const testHelpers = require("../../../utils/js/testHelpers.js"); -const { Transaction, Wallet, Provider, getCreateAddress } = require("ethers"); - -const th = testHelpers.TestHelper; -const toBN = th.toBN; - -const SAVED_DEPLOY_DATA_ZERO = { - serializedDeployTx: - "0xf901f8808403ef14808302194d8080b901a6608060405234801561001057600080fd5b50610186806100206000396000f3fe608060405234801561001057600080fd5b50600436106100365760003560e01c80636981665b1461003b57806369b13ba014610073575b600080fd5b6100616004803603602081101561005157600080fd5b50356001600160a01b0316610094565b60408051918252519081900360200190f35b6100926004803603602081101561008957600080fd5b503515156100a6565b005b60006020819052908152604090205481565b80156100c35732600090815260208190526040902043905561014d565b326000908152602081905260409020541561014d573260009081526020819052604090205443141561013c576040805162461bcd60e51b815260206004820152601f60248201527f5a65726f50726f746f636f6c4d757465783a206d75746578206c6f636b656400604482015290519081900360640190fd5b326000908152602081905260408120555b5056fea26469706673582212208fa44190280703d199dee337f61f207770f627ddf810bdbd4a1c74fcb8b31e2664736f6c634300060b00331ba06d757465786d757465786d757465786d757465786d757465786d757465786d75a06d757465786d757465786d757465786d757465786d757465786d757465786d75", - deployerAddress: "0xb339D675F5Fb2EEec8a13db76dD57C11F4Af3869", - contractAddress: "0x42B023F998d7B9c127e9bDcDCE57ccd1f5e1d919", - transactionCostWei: toBN(9078234000000), -}; - -const getOrDeployZeroProtocolMutex = async () => { - const provider = ethers.provider; - - const { serializedDeployTx, deployerAddress, contractAddress, transactionCostWei } = - SAVED_DEPLOY_DATA_ZERO; - const ZeroProtocolMutex = await ethers.getContractAt("ZeroProtocolMutex", contractAddress); - const deployedCode = await provider.getCode(contractAddress); - if (deployedCode.replace(/0+$/) !== "0x") { - // Contract is deployed - // it's practically impossible to deploy to this address with malicious bytecode so we don't need to check - return ZeroProtocolMutex; - } - - // Not deployed, we need to deploy - console.log("ZeroProtocolMutex has not been deployed, deploying...") - - // Fund the account - const deployerBalance = await provider.getBalance(deployerAddress); - const whale = (await ethers.getSigners())[0]; - if (deployerBalance < transactionCostWei) { - const requiredBalance = toBN(transactionCostWei.toString()).sub(toBN(deployerBalance.toString())); - const tx = await whale.sendTransaction({ - to: deployerAddress, - value: requiredBalance.toString(), - }); - await tx.wait(); - } - - const tx = await provider.broadcastTransaction(serializedDeployTx); - await tx.wait(); - - return ZeroProtocolMutex.attach(contractAddress); -}; - -async function createZeroProtocolMutexDeployTransaction() { - const provider = ethers.provider; - const ZeroProtocolMutex = await ethers.getContractFactory("ZeroProtocolMutex"); - const { data: bytecode } = await ZeroProtocolMutex.getDeployTransaction(); - console.log(bytecode) - - const signature = { - v: 27, // must not be eip-155 to allow cross-chain deployments - // "mutex" in hex: 6d75746578 - // 0xm u t e x m u t e x m u t e x m u t e x m u t e x m u t e x m u - r: "0x6d757465786d757465786d757465786d757465786d757465786d757465786d75", - s: "0x6d757465786d757465786d757465786d757465786d757465786d757465786d75", - }; - - const hardhatGasLimit = await provider.estimateGas({ data: bytecode }); - const gasLimit = toBN(137549); - if (hardhatGasLimit > gasLimit) { - throw new Error( - `Hardhat estimates the gas limit as ${hardhatGasLimit.toString()}, ` + - `which is higher than the hardcoded gas limit ${gasLimit.toString()}` - ); - } - - // 10 gwei, should be enough to also mine on other chains. Could also be 100 like with erc1820 - const gasPrice = toBN(66000000); - - const transactionCostWei = gasLimit.mul(gasPrice); - - const deployTx = { - data: bytecode, // We could hardcode this too - nonce: 0, - gasLimit: gasLimit.toString(), - gasPrice: gasPrice.toString(), - type: 0 - }; - - const transaction = Transaction.from({...deployTx, signature}); - const serializedDeployTx = transaction.serialized; - - console.log(transactionCostWei.toString()) - - const parsedDeployTx = Transaction.from(serializedDeployTx); - const contractAddress = await getCreateAddress({ - from: parsedDeployTx.from, - nonce: parsedDeployTx.nonce, - }); - const deployerAddress = parsedDeployTx.from; - - return { - serializedDeployTx, - deployerAddress, - contractAddress, - transactionCostWei, - }; -} - -module.exports = { - getOrDeployZeroProtocolMutex, - createZeroProtocolMutexDeployTransaction, - SAVED_DEPLOY_DATA_ZERO -}; diff --git a/tests/js/AccessControlTest.js b/tests/js/AccessControlTest.js index f5d9e74..b59055f 100644 --- a/tests/js/AccessControlTest.js +++ b/tests/js/AccessControlTest.js @@ -2,7 +2,6 @@ const deploymentHelper = require("../../utils/js/deploymentHelpers.js"); const testHelpers = require("../../utils/js/testHelpers.js"); const TroveManagerTester = artifacts.require("TroveManagerTester"); const PriceFeedSovryn = artifacts.require("PriceFeedSovrynTester"); -const { getOrDeployZeroProtocolMutex } = require("../../deployment/helpers/reentrancy/utils"); const th = testHelpers.TestHelper; const timeValues = testHelpers.TimeValues; @@ -40,8 +39,6 @@ contract( let communityIssuance; before(async () => { - await getOrDeployZeroProtocolMutex(); - coreContracts = await deploymentHelper.deployLiquityCore(); coreContracts.troveManager = await TroveManagerTester.new(coreContracts.permit2.address); coreContracts = await deploymentHelper.deployZUSDTokenTester(coreContracts); diff --git a/tests/js/BorrowerOperationsTest.js b/tests/js/BorrowerOperationsTest.js index 237bc78..7dee06b 100644 --- a/tests/js/BorrowerOperationsTest.js +++ b/tests/js/BorrowerOperationsTest.js @@ -12,7 +12,6 @@ const MassetManagerTester = artifacts.require("MassetManagerTester"); const NueMockToken = artifacts.require("NueMockToken"); const BorrowerOperationsCrossReentrancy = artifacts.require("BorrowerOperationsCrossReentrancy"); const { AllowanceProvider, PermitTransferFrom, SignatureTransfer } = require("@uniswap/permit2-sdk"); -const { getOrDeployZeroProtocolMutex } = require("../../deployment/helpers/reentrancy/utils"); const th = testHelpers.TestHelper; @@ -186,9 +185,6 @@ contract("BorrowerOperations", async accounts => { let revertToSnapshot; beforeEach(async () => { - // The Mutex singleton must be deployed for SharedReentrancyGuard to work - await getOrDeployZeroProtocolMutex(); - let snapshot = await timeMachine.takeSnapshot(); revertToSnapshot = () => timeMachine.revertToSnapshot(snapshot["result"]); }); diff --git a/tests/js/SharedReentrancyGuardTest.js b/tests/js/SharedReentrancyGuardTest.js deleted file mode 100644 index 1f6c84a..0000000 --- a/tests/js/SharedReentrancyGuardTest.js +++ /dev/null @@ -1,98 +0,0 @@ -const { ethers } = require("hardhat"); -const { getOrDeployZeroProtocolMutex } = require("../../deployment/helpers/reentrancy/utils"); - -describe("SharedReentrancyGuard", async () => { - let nonReentrantValueSetter; - let anotherNonReentrantValueSetter; - let reentrantValueSetter; - let valueSetterProxy; - let proxiedValueSetter; - - beforeEach(async () => { - const NonReentrantValueSetter = await ethers.getContractFactory( - "TestNonReentrantValueSetter" - ); - - const ValueSetter = await ethers.getContractFactory("TestValueSetter"); - const ValueSetterProxy = await ethers.getContractFactory("TestValueSetterProxy"); - nonReentrantValueSetter = await NonReentrantValueSetter.deploy(); - anotherNonReentrantValueSetter = await NonReentrantValueSetter.deploy(); - reentrantValueSetter = await ValueSetter.deploy(); - valueSetterProxy = await ValueSetterProxy.deploy(); - proxiedValueSetter = await ValueSetter.attach(valueSetterProxy.target); - - // The Mutex singleton must be deployed for SharedReentrancyGuard to work - await getOrDeployZeroProtocolMutex(); - }); - - it("sanity check", async () => { - expect(await nonReentrantValueSetter.value()).to.equal(0); - expect(await anotherNonReentrantValueSetter.value()).to.equal(0); - expect(await reentrantValueSetter.value()).to.equal(0); - expect(await valueSetterProxy.value()).to.equal(0); - expect(await proxiedValueSetter.value()).to.equal(0); - - await nonReentrantValueSetter.setValueOpening(1); - await anotherNonReentrantValueSetter.setValueOpening(2); - await reentrantValueSetter.setValueOpening(3); - - await valueSetterProxy.setImplementation(reentrantValueSetter.target); - await proxiedValueSetter.setValueOpening(4); - - expect(await nonReentrantValueSetter.value()).to.equal(1); - expect(await anotherNonReentrantValueSetter.value()).to.equal(2); - expect(await reentrantValueSetter.value()).to.equal(3); - expect(await valueSetterProxy.value()).to.equal(4); - expect(await proxiedValueSetter.value()).to.equal(4); - }); - - it("globallyNonReentrant call from globallyNonReentrant call reverts", async () => { - await expect( - nonReentrantValueSetter.setOtherContractValueNonReentrant( - anotherNonReentrantValueSetter.target, - 1 - ) - ).to.be.revertedWith("ZeroProtocolMutex: mutex locked"); - expect(await anotherNonReentrantValueSetter.value()).to.equal(0); - }); - - it("non-globallyNonReentrant call from globallyNonReentrant call does not revert", async () => { - await nonReentrantValueSetter.setOtherContractValueNonReentrant( - reentrantValueSetter.target, - 1 - ); - expect(await reentrantValueSetter.value()).to.equal(1); - }); - - it("globallyNonReentrant works with proxies", async () => { - await valueSetterProxy.setImplementation(reentrantValueSetter.target); - expect(await proxiedValueSetter.value()).to.equal(0); - - await nonReentrantValueSetter.setOtherContractValueNonReentrant( - valueSetterProxy.target, - 1 - ); - expect(await proxiedValueSetter.value()).to.equal(1); - - await valueSetterProxy.setImplementation(anotherNonReentrantValueSetter.target); - await expect( - nonReentrantValueSetter.setOtherContractValueNonReentrant(valueSetterProxy.target, 2) - ).to.be.revertedWith("ZeroProtocolMutex: mutex locked"); - expect(await proxiedValueSetter.value()).to.equal(1); - - await proxiedValueSetter.setValueOpening(3); - expect(await proxiedValueSetter.value()).to.equal(3); - }); - - it("works with proxies without breaking the memory layout", async () => { - await valueSetterProxy.setImplementation(reentrantValueSetter.target); - expect(await proxiedValueSetter.value()).to.equal(0); - - await proxiedValueSetter.setValueOpening(1); - expect(await proxiedValueSetter.value()).to.equal(1); - - await valueSetterProxy.setImplementation(anotherNonReentrantValueSetter.target); - await proxiedValueSetter.setValueOpening(2); - expect(await proxiedValueSetter.value()).to.equal(2); - }); -}); From 8749bf16938e468c11e21c721267ba74dd7d409d Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Thu, 19 Dec 2024 03:42:32 +0000 Subject: [PATCH 5/9] fixing problems of installation as remote dependency --- package.json | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/package.json b/package.json index f650a8a..d1899ed 100644 --- a/package.json +++ b/package.json @@ -49,12 +49,14 @@ "author": "", "license": "ISC", "dependencies": { + "@openzeppelin/contracts": "^3.3.0", + "@nomicfoundation/hardhat-ethers": "^3.0.4", "cross-env": "^7.0.3", "decimal.js": "^10.2.0", "eth-mutants": "^0.1.1", "eth-permit": "^0.2.3", "ethereumjs-util": "^7.0.9", - "hardhat": "^2.17.2", + "hardhat": "2.17.2", "solc": "^0.6.11", "solhint": "^3.4.0", "solhint-plugin-prettier": "^0.0.5", @@ -63,8 +65,7 @@ "devDependencies": { "@ethersproject/abi": "^5.7.0", "@ethersproject/providers": "^5.7.2", - "@nomicfoundation/hardhat-chai-matchers": "^2.0.2", - "@nomicfoundation/hardhat-ethers": "^3.0.4", + "@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", @@ -72,7 +73,6 @@ "@nomiclabs/hardhat-solhint": "^3.0.1", "@nomiclabs/hardhat-truffle5": "^2.0.0", "@nomiclabs/hardhat-web3": "^2.0.0", - "@openzeppelin/contracts": "^3.3.0", "@openzeppelin/test-helpers": "^0.5.10", "@primitivefi/hardhat-dodoc": "^0.2.3", "@secrez/cryptoenv": "^0.2.4", From bd697a6e2e22bd6012b02197c539832b763f6153 Mon Sep 17 00:00:00 2001 From: cwsnt Date: Thu, 19 Dec 2024 14:57:34 +0700 Subject: [PATCH 6/9] 1/ address comment remove unused test instruction --- contracts/BorrowerOperations.sol | 20 +- contracts/BorrowerOperationsStorage.sol | 4 +- .../BorrowerOperationsCrossReentrancy.sol | 3 - tests/js/BorrowerOperationsTest.js | 836 +++++++++--------- 4 files changed, 430 insertions(+), 433 deletions(-) diff --git a/contracts/BorrowerOperations.sol b/contracts/BorrowerOperations.sol index 129b193..7a4dbd3 100644 --- a/contracts/BorrowerOperations.sol +++ b/contracts/BorrowerOperations.sol @@ -159,20 +159,20 @@ contract BorrowerOperations is } /* - * @notice set the user's block number for opening, and do check & reset to 0 for closing + * @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 notInTheSameBlockHandler(bool _isOpening) private { - if(_isOpening) { - userBlockNumber[tx.origin] = block.number; + function recoveryModeMutexHandler(bool _openOrIncrease) private { + if(_openOrIncrease) { + recoveryModeMutex[msg.sender] = block.number; } else { - if(userBlockNumber[tx.origin] > 0) { - if(userBlockNumber[tx.origin] == block.number) { - revert("ZeroProtocolMutex: mutex locked"); + if(recoveryModeMutex[msg.sender] > 0) { + if(recoveryModeMutex[msg.sender] == block.number) { + revert("Recovery mode mutex locked. Try in another block"); } - userBlockNumber[tx.origin] = 0; + recoveryModeMutex[msg.sender] = 0; } } } @@ -221,7 +221,7 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); bool isRecoveryMode = _checkRecoveryMode(vars.price); - if(isRecoveryMode) notInTheSameBlockHandler(true); + if(isRecoveryMode) recoveryModeMutexHandler(true); _requireValidMaxFeePercentage(_maxFeePercentage, isRecoveryMode); _requireTroveisNotActive(contractsCache.troveManager, msg.sender); @@ -606,7 +606,7 @@ contract BorrowerOperations is vars.isRecoveryMode = _checkRecoveryMode(vars.price); if(vars.isRecoveryMode) { - notInTheSameBlockHandler(_isDebtIncrease); + recoveryModeMutexHandler(_isDebtIncrease); } if (_isDebtIncrease) { diff --git a/contracts/BorrowerOperationsStorage.sol b/contracts/BorrowerOperationsStorage.sol index 6eb7126..658fa10 100644 --- a/contracts/BorrowerOperationsStorage.sol +++ b/contracts/BorrowerOperationsStorage.sol @@ -38,7 +38,7 @@ contract BorrowerOperationsStorage is Ownable { IFeeDistributor public feeDistributor; /* - * to store the user's block number + * Store the LoC block number on open/increase when in the Recovery mode */ - mapping(address => uint256) public userBlockNumber; + mapping(address => uint256) public recoveryModeMutex; } diff --git a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol index f3452d6..911d180 100644 --- a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol +++ b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol @@ -32,9 +32,6 @@ contract BorrowerOperationsCrossReentrancy { _lowerHint ); - // manipulate the price so that the recovery mode will be triggered - IPriceFeedTestnet(_priceFeed).setPrice(1e8); - // // should revert due to reentrancy violation borrowerOperations.addColl(_upperHint, _lowerHint); } diff --git a/tests/js/BorrowerOperationsTest.js b/tests/js/BorrowerOperationsTest.js index 7dee06b..f1f7277 100644 --- a/tests/js/BorrowerOperationsTest.js +++ b/tests/js/BorrowerOperationsTest.js @@ -6579,423 +6579,423 @@ contract("BorrowerOperations", async accounts => { // --- getNewTCRFromTroveChange - (external wrapper in Tester contract calls internal function) --- describe("getNewTCRFromTroveChange() returns the correct TCR", async () => { - // 0, 0 - it("collChange = 0, debtChange = 0", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, "ether")); - const troveTotalDebt = toBN(dec(100000, 18)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - - // --- TEST --- - const collChange = 0; - const debtChange = 0; - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - true, - debtChange, - true, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt)); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // 0, +ve - it("collChange = 0, debtChange is positive", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, "ether")); - const troveTotalDebt = toBN(dec(100000, 18)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - - // --- TEST --- - const collChange = 0; - const debtChange = dec(200, 18); - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - true, - debtChange, - true, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // 0, -ve - it("collChange = 0, debtChange is negative", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, "ether")); - const troveTotalDebt = toBN(dec(100000, 18)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - // --- TEST --- - const collChange = 0; - const debtChange = dec(100, 16); - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - true, - debtChange, - false, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // +ve, 0 - it("collChange is positive, debtChange is 0", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, "ether")); - const troveTotalDebt = toBN(dec(100000, 18)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - // --- TEST --- - const collChange = dec(2, "ether"); - const debtChange = 0; - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - true, - debtChange, - true, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .add(toBN(collChange)) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt)); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // -ve, 0 - it("collChange is negative, debtChange is 0", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, 16)); - const troveTotalDebt = toBN(dec(100000, 16)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - - // --- TEST --- - const collChange = dec(1, 16); - const debtChange = 0; - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - false, - debtChange, - true, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .sub(toBN(dec(1, 16))) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt)); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // -ve, -ve - it("collChange is negative, debtChange is negative", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, 16)); - const troveTotalDebt = toBN(dec(100000, 16)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - - // --- TEST --- - const collChange = dec(1, 16); - const debtChange = dec(100, 16); - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - false, - debtChange, - false, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .sub(toBN(dec(1, 16))) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // +ve, +ve - it("collChange is positive, debtChange is positive", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, "ether")); - const troveTotalDebt = toBN(dec(100000, 18)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - - // --- TEST --- - const collChange = dec(1, 16); - const debtChange = dec(100, 16); - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - true, - debtChange, - true, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .add(toBN(dec(1, 16))) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt).add(toBN(dec(100, 16)))); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // +ve, -ve - it("collChange is positive, debtChange is negative", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, "ether")); - const troveTotalDebt = toBN(dec(100000, 18)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - - // --- TEST --- - const collChange = dec(1, 16); - const debtChange = dec(100, 16); - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - true, - debtChange, - false, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .add(toBN(dec(1, 16))) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); - - // -ve, +ve - it("collChange is negative, debtChange is positive", async () => { - // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - const troveColl = toBN(dec(1000, "ether")); - const troveTotalDebt = toBN(dec(100000, 18)); - const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - from: alice, - value: troveColl - }); - await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - from: bob, - value: troveColl - }); - - await priceFeed.setPrice(dec(100, 18)); - - const liquidationTx = await troveManager.liquidate(bob); - assert.isFalse(await sortedTroves.contains(bob)); - - const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - liquidationTx - ); - - await priceFeed.setPrice(dec(200, 18)); - const price = await priceFeed.getPrice(); - - // --- TEST --- - const collChange = dec(1, 18); - const debtChange = await getNetBorrowingAmount(dec(200, 18)); - const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - collChange, - false, - debtChange, - true, - price - ); - - const expectedTCR = troveColl - .add(liquidatedColl) - .sub(toBN(collChange)) - .mul(price) - .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); - - assert.isTrue(newTCR.eq(expectedTCR)); - }); + // // 0, 0 + // it("collChange = 0, debtChange = 0", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, "ether")); + // const troveTotalDebt = toBN(dec(100000, 18)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + + // // --- TEST --- + // const collChange = 0; + // const debtChange = 0; + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // true, + // debtChange, + // true, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt)); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // 0, +ve + // it("collChange = 0, debtChange is positive", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, "ether")); + // const troveTotalDebt = toBN(dec(100000, 18)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + + // // --- TEST --- + // const collChange = 0; + // const debtChange = dec(200, 18); + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // true, + // debtChange, + // true, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // 0, -ve + // it("collChange = 0, debtChange is negative", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, "ether")); + // const troveTotalDebt = toBN(dec(100000, 18)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + // // --- TEST --- + // const collChange = 0; + // const debtChange = dec(100, 16); + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // true, + // debtChange, + // false, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // +ve, 0 + // it("collChange is positive, debtChange is 0", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, "ether")); + // const troveTotalDebt = toBN(dec(100000, 18)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + // // --- TEST --- + // const collChange = dec(2, "ether"); + // const debtChange = 0; + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // true, + // debtChange, + // true, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .add(toBN(collChange)) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt)); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // -ve, 0 + // it("collChange is negative, debtChange is 0", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, 16)); + // const troveTotalDebt = toBN(dec(100000, 16)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + + // // --- TEST --- + // const collChange = dec(1, 16); + // const debtChange = 0; + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // false, + // debtChange, + // true, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .sub(toBN(dec(1, 16))) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt)); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // -ve, -ve + // it("collChange is negative, debtChange is negative", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, 16)); + // const troveTotalDebt = toBN(dec(100000, 16)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + + // // --- TEST --- + // const collChange = dec(1, 16); + // const debtChange = dec(100, 16); + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // false, + // debtChange, + // false, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .sub(toBN(dec(1, 16))) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // +ve, +ve + // it("collChange is positive, debtChange is positive", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, "ether")); + // const troveTotalDebt = toBN(dec(100000, 18)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + + // // --- TEST --- + // const collChange = dec(1, 16); + // const debtChange = dec(100, 16); + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // true, + // debtChange, + // true, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .add(toBN(dec(1, 16))) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt).add(toBN(dec(100, 16)))); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // +ve, -ve + // it("collChange is positive, debtChange is negative", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, "ether")); + // const troveTotalDebt = toBN(dec(100000, 18)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + + // // --- TEST --- + // const collChange = dec(1, 16); + // const debtChange = dec(100, 16); + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // true, + // debtChange, + // false, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .add(toBN(dec(1, 16))) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); + + // // -ve, +ve + // it("collChange is negative, debtChange is positive", async () => { + // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + // const troveColl = toBN(dec(1000, "ether")); + // const troveTotalDebt = toBN(dec(100000, 18)); + // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + // from: alice, + // value: troveColl + // }); + // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + // from: bob, + // value: troveColl + // }); + + // await priceFeed.setPrice(dec(100, 18)); + + // const liquidationTx = await troveManager.liquidate(bob); + // assert.isFalse(await sortedTroves.contains(bob)); + + // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + // liquidationTx + // ); + + // await priceFeed.setPrice(dec(200, 18)); + // const price = await priceFeed.getPrice(); + + // // --- TEST --- + // const collChange = dec(1, 18); + // const debtChange = await getNetBorrowingAmount(dec(200, 18)); + // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + // collChange, + // false, + // debtChange, + // true, + // price + // ); + + // const expectedTCR = troveColl + // .add(liquidatedColl) + // .sub(toBN(collChange)) + // .mul(price) + // .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); + + // assert.isTrue(newTCR.eq(expectedTCR)); + // }); it("openTrove(): open a Trove, then adjust it in the same block should revert", async () => { await openTrove({ ICR: toBN(dec(2, 18)), extraParams: { from: alice } }); @@ -7022,7 +7022,7 @@ contract("BorrowerOperations", async accounts => { whale, priceFeed.address, {value: ICR.mul(totalDebt).div(price)}), - "ZeroProtocolMutex: mutex locked" + "Recovery mode mutex locked. Try in another block" ); }); }); From 8b700235ef8541bb04d919927c288c17041e6608 Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Fri, 20 Dec 2024 00:54:15 +0000 Subject: [PATCH 7/9] fixing problems of installation as remote dependency --- package.json | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 From 87907c748eda0da187bd5f5ed5f48f5d4c511b4e Mon Sep 17 00:00:00 2001 From: cwsnt Date: Fri, 27 Dec 2024 15:30:04 +0700 Subject: [PATCH 8/9] fix: unit test 2 address comment review --- contracts/BorrowerOperations.sol | 6 +- .../BorrowerOperationsCrossReentrancy.sol | 30 +- tests/js/BorrowerOperationsTest.js | 869 +++++++++--------- 3 files changed, 479 insertions(+), 426 deletions(-) diff --git a/contracts/BorrowerOperations.sol b/contracts/BorrowerOperations.sol index 7a4dbd3..c80a9f7 100644 --- a/contracts/BorrowerOperations.sol +++ b/contracts/BorrowerOperations.sol @@ -221,7 +221,9 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); bool isRecoveryMode = _checkRecoveryMode(vars.price); - if(isRecoveryMode) recoveryModeMutexHandler(true); + if(isRecoveryMode && _ZUSDAmount > 0) { + recoveryModeMutexHandler(true); + } _requireValidMaxFeePercentage(_maxFeePercentage, isRecoveryMode); _requireTroveisNotActive(contractsCache.troveManager, msg.sender); @@ -605,7 +607,7 @@ contract BorrowerOperations is vars.price = priceFeed.fetchPrice(); vars.isRecoveryMode = _checkRecoveryMode(vars.price); - if(vars.isRecoveryMode) { + if(vars.isRecoveryMode && _ZUSDChange > 0) { recoveryModeMutexHandler(_isDebtIncrease); } diff --git a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol index 911d180..22bcb54 100644 --- a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol +++ b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol @@ -18,7 +18,28 @@ contract BorrowerOperationsCrossReentrancy { fallback() external payable {} - function testCrossReentrancy( + 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); + } + + function testCrossReentrancyAffectingDebt( uint256 _maxFeePercentage, uint256 _ZUSDAmount, address _upperHint, @@ -32,7 +53,10 @@ contract BorrowerOperationsCrossReentrancy { _lowerHint ); - // // should revert due to reentrancy violation - borrowerOperations.addColl(_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); } } diff --git a/tests/js/BorrowerOperationsTest.js b/tests/js/BorrowerOperationsTest.js index f1f7277..60f1918 100644 --- a/tests/js/BorrowerOperationsTest.js +++ b/tests/js/BorrowerOperationsTest.js @@ -6579,425 +6579,452 @@ contract("BorrowerOperations", async accounts => { // --- getNewTCRFromTroveChange - (external wrapper in Tester contract calls internal function) --- describe("getNewTCRFromTroveChange() returns the correct TCR", async () => { - // // 0, 0 - // it("collChange = 0, debtChange = 0", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, "ether")); - // const troveTotalDebt = toBN(dec(100000, 18)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - - // // --- TEST --- - // const collChange = 0; - // const debtChange = 0; - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // true, - // debtChange, - // true, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt)); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // 0, +ve - // it("collChange = 0, debtChange is positive", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, "ether")); - // const troveTotalDebt = toBN(dec(100000, 18)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - - // // --- TEST --- - // const collChange = 0; - // const debtChange = dec(200, 18); - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // true, - // debtChange, - // true, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // 0, -ve - // it("collChange = 0, debtChange is negative", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, "ether")); - // const troveTotalDebt = toBN(dec(100000, 18)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - // // --- TEST --- - // const collChange = 0; - // const debtChange = dec(100, 16); - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // true, - // debtChange, - // false, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // +ve, 0 - // it("collChange is positive, debtChange is 0", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, "ether")); - // const troveTotalDebt = toBN(dec(100000, 18)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - // // --- TEST --- - // const collChange = dec(2, "ether"); - // const debtChange = 0; - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // true, - // debtChange, - // true, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .add(toBN(collChange)) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt)); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // -ve, 0 - // it("collChange is negative, debtChange is 0", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, 16)); - // const troveTotalDebt = toBN(dec(100000, 16)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - - // // --- TEST --- - // const collChange = dec(1, 16); - // const debtChange = 0; - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // false, - // debtChange, - // true, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .sub(toBN(dec(1, 16))) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt)); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // -ve, -ve - // it("collChange is negative, debtChange is negative", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, 16)); - // const troveTotalDebt = toBN(dec(100000, 16)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - - // // --- TEST --- - // const collChange = dec(1, 16); - // const debtChange = dec(100, 16); - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // false, - // debtChange, - // false, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .sub(toBN(dec(1, 16))) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // +ve, +ve - // it("collChange is positive, debtChange is positive", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, "ether")); - // const troveTotalDebt = toBN(dec(100000, 18)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - - // // --- TEST --- - // const collChange = dec(1, 16); - // const debtChange = dec(100, 16); - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // true, - // debtChange, - // true, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .add(toBN(dec(1, 16))) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt).add(toBN(dec(100, 16)))); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // +ve, -ve - // it("collChange is positive, debtChange is negative", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, "ether")); - // const troveTotalDebt = toBN(dec(100000, 18)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - - // // --- TEST --- - // const collChange = dec(1, 16); - // const debtChange = dec(100, 16); - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // true, - // debtChange, - // false, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .add(toBN(dec(1, 16))) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - // // -ve, +ve - // it("collChange is negative, debtChange is positive", async () => { - // // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) - // const troveColl = toBN(dec(1000, "ether")); - // const troveTotalDebt = toBN(dec(100000, 18)); - // const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { - // from: alice, - // value: troveColl - // }); - // await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { - // from: bob, - // value: troveColl - // }); - - // await priceFeed.setPrice(dec(100, 18)); - - // const liquidationTx = await troveManager.liquidate(bob); - // assert.isFalse(await sortedTroves.contains(bob)); - - // const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( - // liquidationTx - // ); - - // await priceFeed.setPrice(dec(200, 18)); - // const price = await priceFeed.getPrice(); - - // // --- TEST --- - // const collChange = dec(1, 18); - // const debtChange = await getNetBorrowingAmount(dec(200, 18)); - // const newTCR = await borrowerOperations.getNewTCRFromTroveChange( - // collChange, - // false, - // debtChange, - // true, - // price - // ); - - // const expectedTCR = troveColl - // .add(liquidatedColl) - // .sub(toBN(collChange)) - // .mul(price) - // .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); - - // assert.isTrue(newTCR.eq(expectedTCR)); - // }); - - it("openTrove(): open a Trove, then adjust it in the same block should revert", async () => { + // 0, 0 + it("collChange = 0, debtChange = 0", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, "ether")); + const troveTotalDebt = toBN(dec(100000, 18)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + + // --- TEST --- + const collChange = 0; + const debtChange = 0; + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + true, + debtChange, + true, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt)); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // 0, +ve + it("collChange = 0, debtChange is positive", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, "ether")); + const troveTotalDebt = toBN(dec(100000, 18)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + + // --- TEST --- + const collChange = 0; + const debtChange = dec(200, 18); + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + true, + debtChange, + true, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // 0, -ve + it("collChange = 0, debtChange is negative", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, "ether")); + const troveTotalDebt = toBN(dec(100000, 18)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + // --- TEST --- + const collChange = 0; + const debtChange = dec(100, 16); + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + true, + debtChange, + false, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // +ve, 0 + it("collChange is positive, debtChange is 0", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, "ether")); + const troveTotalDebt = toBN(dec(100000, 18)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + // --- TEST --- + const collChange = dec(2, "ether"); + const debtChange = 0; + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + true, + debtChange, + true, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .add(toBN(collChange)) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt)); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // -ve, 0 + it("collChange is negative, debtChange is 0", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, 16)); + const troveTotalDebt = toBN(dec(100000, 16)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + + // --- TEST --- + const collChange = dec(1, 16); + const debtChange = 0; + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + false, + debtChange, + true, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .sub(toBN(dec(1, 16))) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt)); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // -ve, -ve + it("collChange is negative, debtChange is negative", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, 16)); + const troveTotalDebt = toBN(dec(100000, 16)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + + // --- TEST --- + const collChange = dec(1, 16); + const debtChange = dec(100, 16); + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + false, + debtChange, + false, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .sub(toBN(dec(1, 16))) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // +ve, +ve + it("collChange is positive, debtChange is positive", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, "ether")); + const troveTotalDebt = toBN(dec(100000, 18)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + + // --- TEST --- + const collChange = dec(1, 16); + const debtChange = dec(100, 16); + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + true, + debtChange, + true, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .add(toBN(dec(1, 16))) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt).add(toBN(dec(100, 16)))); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // +ve, -ve + it("collChange is positive, debtChange is negative", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, "ether")); + const troveTotalDebt = toBN(dec(100000, 18)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + + // --- TEST --- + const collChange = dec(1, 16); + const debtChange = dec(100, 16); + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + true, + debtChange, + false, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .add(toBN(dec(1, 16))) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt).sub(toBN(dec(100, 16)))); + + assert.isTrue(newTCR.eq(expectedTCR)); + }); + + // -ve, +ve + it("collChange is negative, debtChange is positive", async () => { + // --- SETUP --- Create a Zero instance with an Active Pool and pending rewards (Default Pool) + const troveColl = toBN(dec(1000, "ether")); + const troveTotalDebt = toBN(dec(100000, 18)); + const troveZUSDAmount = await getOpenTroveZUSDAmount(troveTotalDebt); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, alice, alice, { + from: alice, + value: troveColl + }); + await borrowerOperations.openTrove(th._100pct, troveZUSDAmount, bob, bob, { + from: bob, + value: troveColl + }); + + await priceFeed.setPrice(dec(100, 18)); + + const liquidationTx = await troveManager.liquidate(bob); + assert.isFalse(await sortedTroves.contains(bob)); + + const [liquidatedDebt, liquidatedColl, gasComp] = th.getEmittedLiquidationValues( + liquidationTx + ); + + await priceFeed.setPrice(dec(200, 18)); + const price = await priceFeed.getPrice(); + + // --- TEST --- + const collChange = dec(1, 18); + const debtChange = await getNetBorrowingAmount(dec(200, 18)); + const newTCR = await borrowerOperations.getNewTCRFromTroveChange( + collChange, + false, + debtChange, + true, + price + ); + + const expectedTCR = troveColl + .add(liquidatedColl) + .sub(toBN(collChange)) + .mul(price) + .div(troveTotalDebt.add(liquidatedDebt).add(toBN(debtChange))); + + 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"); @@ -7015,13 +7042,13 @@ contract("BorrowerOperations", async accounts => { const borrowerOperationsCrossReentrancy = await BorrowerOperationsCrossReentrancy.new(contracts.borrowerOperations.address); await assertRevert( - borrowerOperationsCrossReentrancy.testCrossReentrancy( + borrowerOperationsCrossReentrancy.testCrossReentrancyAffectingDebt( th._100pct, extraZUSDAmount, whale, whale, priceFeed.address, - {value: ICR.mul(totalDebt).div(price)}), + {value: ICR.mul(totalDebt).div(price).mul(toBN(2))}), "Recovery mode mutex locked. Try in another block" ); }); From a55672a648c46e9a3e5c75f0aba2cc6243217d10 Mon Sep 17 00:00:00 2001 From: cwsnt Date: Tue, 31 Dec 2024 14:45:09 +0700 Subject: [PATCH 9/9] add unit test --- .../BorrowerOperationsCrossReentrancy.sol | 27 ++++++++++++ tests/js/BorrowerOperationsTest.js | 42 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol index 22bcb54..fdd3f4d 100644 --- a/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol +++ b/contracts/TestContracts/BorrowerOperationsCrossReentrancy.sol @@ -2,6 +2,7 @@ pragma solidity 0.6.11; import "../Interfaces/IBorrowerOperations.sol"; +import "hardhat/console.sol"; interface IPriceFeedTestnet { function setPrice(uint256 price) external returns (bool); @@ -39,6 +40,9 @@ contract BorrowerOperationsCrossReentrancy { 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, @@ -59,4 +63,27 @@ contract BorrowerOperationsCrossReentrancy { // 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/tests/js/BorrowerOperationsTest.js b/tests/js/BorrowerOperationsTest.js index 60f1918..17f1f72 100644 --- a/tests/js/BorrowerOperationsTest.js +++ b/tests/js/BorrowerOperationsTest.js @@ -7052,6 +7052,48 @@ contract("BorrowerOperations", async accounts => { "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) {