From 92cf66f23f335594e918dfb931716e9c88b3bbe3 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Wed, 2 Apr 2025 12:22:14 +1100 Subject: [PATCH 01/12] Protocol fee update (#137) * update view and local tests * add changelog * minor --- CHANGELOG.md | 14 ++++++ src/CollarProviderNFT.sol | 50 ++++++++++++++++--- src/Rolls.sol | 3 +- .../full-protocol/BaseAssetPairForkTest.sol | 2 +- test/unit/CollarProviderNFT.t.sol | 26 +++++----- test/unit/Loans.basic.effects.t.sol | 2 +- test/unit/Rolls.t.sol | 2 +- 7 files changed, 76 insertions(+), 23 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..35a334e7 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,14 @@ +# Changelog + +Notable changes to contracts. + +# Unreleased + +- CollarProviderNFT: + - `protocolFee` charges fee on full notional amount (cash value of loan input), and expects `callStrikePercent` argument. +- Rolls: + - Update to use the updated protocolFee interface. + +# 0.2.0 (All contracts) + +First version deployed to Base mainnet. diff --git a/src/CollarProviderNFT.sol b/src/CollarProviderNFT.sol index 4ed18733..957daf7a 100644 --- a/src/CollarProviderNFT.sol +++ b/src/CollarProviderNFT.sol @@ -126,15 +126,48 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { }); } - /// @notice Calculates the protocol fee charged from offers on position creation. - /// @dev fee is set to 0 if recipient is zero because no transfer will be done - function protocolFee(uint providerLocked, uint duration) public view returns (uint fee, address to) { + /** + * @notice Calculates the protocol fee charged from offers on position creation. + * The fee is charged on the full notional value of the underlying's cash value - i.e. on 100%. + * Example: If providerLocked is 100, and callsTrike is 110%, the notional is 1000. If the APR is 1%, + * so the fee will on 1 year duration will be 1% of 1000 = 10, **on top** of the providerLocked. + * So when position is created, the offer amount will be reduced by 100 + 10 in this example, + * with 100 in providerLocked, and 10 sent to protocol fee recipient. + * @dev fee is set to 0 if recipient is zero because no transfer will be done + */ + function protocolFee(uint providerLocked, uint duration, uint callStrikePercent) + public + view + returns (uint fee, address to) + { to = configHub.feeRecipient(); + // prevent non-zero fee to zero-recipient. + if (to == address(0)) return (0, to); + uint apr = configHub.protocolFeeAPR(); require(apr <= MAX_PROTOCOL_FEE_BIPS, "provider: protocol fee APR too high"); - // prevents non-zero fee to zero-recipient. + + /* Calculation explanation: + + 1. fullNotional = providerLocked * BIPS_BASE / (callStrikePercent - BIPS_BASE) + - providerLocked was proportionally calculated from callStrikePercent + in CollarTakerNFT.calculateProviderLocked from takerLocked + - takerLocked was proportional to putStrikePercent in LoansNFT._swapAndMintCollar + So to get to full notional that was used in LoanNFT we need to divide providerLocked by + (callStrikePercent - 100%). + + 2. Apply the APR: + fee = fullNotional * feeAPR * duration / (BIPS_BASE * YEAR) + + 3. Combine 1 and 2: + fee = providerLocked * BIPS_BASE * feeAPR * duration / ((callStrikePercent - BIPS_BASE) * BIPS_BASE * YEAR) + + 4. Simplify BIPS_BASE: + fee = providerLocked * feeAPR * duration / ((callStrikePercent - BIPS_BASE) * YEAR + */ + // rounds up to prevent avoiding fee using many small positions. - fee = to == address(0) ? 0 : Math.ceilDiv(providerLocked * apr * duration, BIPS_BASE * YEAR); + fee = Math.ceilDiv(providerLocked * apr * duration, (callStrikePercent - BIPS_BASE) * YEAR); } // ----- MUTATIVE ----- // @@ -223,8 +256,10 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { /// @notice Mints a new position from an existing offer. Can ONLY be called through the /// taker contract, which is trusted to open and settle the offer according to the terms. /// Offer parameters are checked vs. the global config to ensure they are still supported. - /// Protocol fee (based on the provider amount and duration) is deducted from offer, and sent + /// Protocol fee (charged on full notional value of the underlying) is deducted from offer, and sent /// to fee recipient. Offer amount is updated as well. + /// Note that because of how protocol fee is calculated, for low callStrikes it can be higher + /// than providerLocked itself. See protocolFee view for calculation details. /// The NFT, representing ownership of the position, is minted to original provider of the offer. /// @param offerId The ID of the offer to mint from /// @param providerLocked The amount of cash asset to use for the new position @@ -248,7 +283,8 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { require(configHub.isValidCollarDuration(offer.duration), "provider: unsupported duration"); // calc protocol fee to subtract from offer (on top of amount) - (uint fee, address feeRecipient) = protocolFee(providerLocked, offer.duration); + (uint fee, address feeRecipient) = + protocolFee(providerLocked, offer.duration, offer.callStrikePercent); // check amount require(providerLocked >= offer.minLocked, "provider: amount too low"); diff --git a/src/Rolls.sol b/src/Rolls.sol index 35cc4f77..f67651e9 100644 --- a/src/Rolls.sol +++ b/src/Rolls.sol @@ -398,7 +398,8 @@ contract Rolls is IRolls, BaseManaged { (uint newTakerLocked, uint newProviderLocked) = _newLockedAmounts(takerPos, newPrice); // new protocol fee. @dev there is no refund for previously paid protocol fee - (uint protocolFee,) = takerPos.providerNFT.protocolFee(newProviderLocked, takerPos.duration); + (uint protocolFee,) = + takerPos.providerNFT.protocolFee(newProviderLocked, takerPos.duration, takerPos.callStrikePercent); // The taker and provider external balances (before fee) should be updated as if // they settled and withdrawn the old positions, and opened the new positions. diff --git a/test/integration/full-protocol/BaseAssetPairForkTest.sol b/test/integration/full-protocol/BaseAssetPairForkTest.sol index 9a1e9041..c9587786 100644 --- a/test/integration/full-protocol/BaseAssetPairForkTest.sol +++ b/test/integration/full-protocol/BaseAssetPairForkTest.sol @@ -319,7 +319,7 @@ abstract contract BaseAssetPairForkTest is Test { // Calculate protocol fee based on post-swap provider locked amount uint swapOut = loanAmount * BIPS_BASE / ltv; uint initProviderLocked = swapOut * (callstrikeToUse - BIPS_BASE) / BIPS_BASE; - (protocolFee,) = pair.providerNFT.protocolFee(initProviderLocked, duration); + (protocolFee,) = pair.providerNFT.protocolFee(initProviderLocked, duration, callstrikeToUse); assertGt(protocolFee, 0); } diff --git a/test/unit/CollarProviderNFT.t.sol b/test/unit/CollarProviderNFT.t.sol index 88d2853a..339bcf16 100644 --- a/test/unit/CollarProviderNFT.t.sol +++ b/test/unit/CollarProviderNFT.t.sol @@ -58,7 +58,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { // calculate expected uint numer = positionAmount * protocolFeeAPR * duration; // round up = ((x - 1) / y) + 1 - expectedFee = numer == 0 ? 0 : (1 + ((numer - 1) / BIPS_100PCT / 365 days)); + expectedFee = numer == 0 ? 0 : (1 + ((numer - 1) / (callStrikePercent - BIPS_100PCT) / 365 days)); // no fee for 0 recipient expectedFee = (configHub.feeRecipient() == address(0)) ? 0 : expectedFee; if (positionAmount != 0 && expectNonZeroFee) { @@ -66,7 +66,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { assertTrue(expectedFee > 0); } // check the view - (uint feeView, address toView) = providerNFT.protocolFee(positionAmount, duration); + (uint feeView, address toView) = providerNFT.protocolFee(positionAmount, duration, callStrikePercent); assertEq(feeView, expectedFee); assertEq(toView, configHub.feeRecipient()); } @@ -347,7 +347,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { // zero recipient, non-zero fee (prevented by config-hub setter, so needs to be mocked) vm.mockCall(address(configHub), abi.encodeCall(configHub.feeRecipient, ()), abi.encode(address(0))); - (uint fee, address feeRecipient) = providerNFT.protocolFee(amount, duration); + (uint fee, address feeRecipient) = providerNFT.protocolFee(amount, duration, callStrikePercent); assertEq(fee, 0); assertEq(feeRecipient, address(0)); // this value is checked in the createAndCheckPosition helper to be deducted @@ -365,27 +365,29 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { // zero APR vm.startPrank(owner); configHub.setProtocolFeeParams(0, protocolFeeRecipient); - (uint fee, address feeRecipient) = providerNFT.protocolFee(largeCash, duration); + (uint fee, address feeRecipient) = providerNFT.protocolFee(largeCash, duration, callStrikePercent); assertEq(feeRecipient, protocolFeeRecipient); assertEq(fee, 0); // test round up configHub.setProtocolFeeParams(1, feeRecipient); - (fee,) = providerNFT.protocolFee(1, 1); // very small fee + (fee,) = providerNFT.protocolFee(1, 1, callStrikePercent); // very small fee assertEq(fee, 1); // rounded up // test zero numerator - (fee,) = providerNFT.protocolFee(0, 1); + (fee,) = providerNFT.protocolFee(0, 1, callStrikePercent); assertEq(fee, 0); - (fee,) = providerNFT.protocolFee(1, 0); + (fee,) = providerNFT.protocolFee(1, 0, callStrikePercent); assertEq(fee, 0); // check calculation for specific hardcoded value configHub.setProtocolFeeParams(50, feeRecipient); // 0.5% per year - (fee,) = providerNFT.protocolFee(cashUnits(10), 365 days); - assertEq(fee, cashFraction(0.05 ether)); - (fee,) = providerNFT.protocolFee(cashUnits(10) + 1, 365 days); - assertEq(fee, cashFraction(0.05 ether) + 1); // rounds up + (fee,) = providerNFT.protocolFee(cashUnits(10), 365 days, 12_000); + // 10 cash providerLocked, at 120% call strike, means 50 notional + // 0.5% on 50 notional is 0.25 + assertEq(fee, cashFraction(0.25 ether)); + (fee,) = providerNFT.protocolFee(cashUnits(10) + 1, 365 days, 12_000); + assertEq(fee, cashFraction(0.25 ether) + 1); // rounds up } function test_settlePositionIncrease() public { @@ -754,7 +756,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { vm.expectRevert("provider: protocol fee APR too high"); providerNFT.mintFromOffer(offerId, largeCash, 0); vm.expectRevert("provider: protocol fee APR too high"); - providerNFT.protocolFee(offerId, 0); + providerNFT.protocolFee(offerId, 0, callStrikePercent); } function test_revert_settlePosition() public { diff --git a/test/unit/Loans.basic.effects.t.sol b/test/unit/Loans.basic.effects.t.sol index 96c109f7..89507fa0 100644 --- a/test/unit/Loans.basic.effects.t.sol +++ b/test/unit/Loans.basic.effects.t.sol @@ -182,7 +182,7 @@ contract LoansTestBase is BaseAssetPairTestSetup { uint expectedLoanAmount = swapOut * ltv / BIPS_100PCT; uint expectedProviderLocked = swapOut * (callStrikePercent - BIPS_100PCT) / BIPS_100PCT; - (uint expectedProtocolFee,) = providerNFT.protocolFee(expectedProviderLocked, duration); + (uint expectedProtocolFee,) = providerNFT.protocolFee(expectedProviderLocked, duration, callStrikePercent); if (expectedProviderLocked != 0) assertGt(expectedProtocolFee, 0); // ensure fee is expected ILoansNFT.SwapParams memory swapParams = defaultSwapParams(swapCashAmount); diff --git a/test/unit/Rolls.t.sol b/test/unit/Rolls.t.sol index 27fb68ce..42f7daed 100644 --- a/test/unit/Rolls.t.sol +++ b/test/unit/Rolls.t.sol @@ -131,7 +131,7 @@ contract RollsTest is BaseAssetPairTestSetup { takerNFT.calculateProviderLocked(expected.newTakerLocked, ltv, callStrikePercent) ); // protocol fee - (expected.toProtocol,) = providerNFT.protocolFee(expected.newProviderLocked, duration); + (expected.toProtocol,) = providerNFT.protocolFee(expected.newProviderLocked, duration, callStrikePercent); // _calculateTransferAmounts (uint takerSettled, int providerChange) = takerNFT.previewSettlement(oldTakerPos, newPrice); int providerSettled = int(oldTakerPos.providerLocked) + providerChange; From bbbe75d565468c0c65129b5f90c38960cf6b5ee8 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Wed, 2 Apr 2025 12:23:51 +1100 Subject: [PATCH 02/12] Ownership reduction part 1 - remove pausing and ownership (#138) * update view and local tests * add changelog * minor * remove pausing and ownership in basemanaged * remove pausing and ownership for rolls * remove rest of pausing and ownership, fix tests * update changelog * update version string * add version string check in fork tests --- CHANGELOG.md | 10 +- script/deploy/accept-ownership.s.sol | 2 +- script/libraries/ArbitrumMainnetDeployer.sol | 12 +- script/libraries/ArbitrumSepoliaDeployer.sol | 8 +- script/libraries/BaseDeployer.sol | 53 ++----- script/libraries/OPBaseMainnetDeployer.sol | 8 +- script/libraries/OPBaseSepoliaDeployer.sol | 8 +- src/CollarProviderNFT.sol | 16 +- src/CollarTakerNFT.sol | 26 +-- src/EscrowSupplierNFT.sol | 26 ++- src/LoansNFT.sol | 30 ++-- src/Rolls.sol | 22 +-- src/base/BaseManaged.sol | 76 ++++----- src/base/BaseNFT.sol | 13 +- .../full-protocol/AssetDeployForkTest.t.sol | 1 - .../full-protocol/BaseAssetPairForkTest.sol | 16 +- test/unit/BaseAssetPairTestSetup.sol | 11 +- test/unit/BaseManaged.all.t.sol | 149 ++++-------------- test/unit/CollarProviderNFT.t.sol | 51 +----- test/unit/CollarTakerNFT.t.sol | 70 ++------ test/unit/EscrowSupplier.admin.t.sol | 57 +------ test/unit/EscrowSupplierNFT.effects.t.sol | 8 +- test/unit/Loans.admin.t.sol | 56 +------ test/unit/Loans.basic.effects.t.sol | 13 +- test/unit/Loans.escrow.reverts.t.sol | 2 +- test/unit/Rolls.t.sol | 42 +---- 26 files changed, 198 insertions(+), 588 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35a334e7..49278f18 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,20 @@ Notable changes to contracts. -# Unreleased +# Unreleased (0.3.0) +- BaseManaged: + - No direct ownership, instead has `onlyConfigHubOwner` modifier. + - All pausing removed. +- CollarTakerNFT, LoansNFT, EscrowSupplierNFT, CollarProviderNFT: + - Inheriting from updated BaseManaged (not ownable, not pausable). + - Different constructor interface. + - Using `onlyConfigHubOwner` for admin methods. - CollarProviderNFT: - `protocolFee` charges fee on full notional amount (cash value of loan input), and expects `callStrikePercent` argument. - Rolls: - Update to use the updated protocolFee interface. + - Not inheriting from BaseManaged. # 0.2.0 (All contracts) diff --git a/script/deploy/accept-ownership.s.sol b/script/deploy/accept-ownership.s.sol index 376b9226..3609ace0 100644 --- a/script/deploy/accept-ownership.s.sol +++ b/script/deploy/accept-ownership.s.sol @@ -48,7 +48,7 @@ abstract contract AcceptOwnershipScript is Script { DeploymentArtifactsLib.loadHubAndAllPairs(vm, artifactsName); // deploy and nominate owner - BaseDeployer.acceptOwnershipAsSender(owner, result.configHub, result.assetPairContracts); + BaseDeployer.acceptOwnershipAsSender(owner, result.configHub); vm.stopBroadcast(); } diff --git a/script/libraries/ArbitrumMainnetDeployer.sol b/script/libraries/ArbitrumMainnetDeployer.sol index 9d376274..3a417155 100644 --- a/script/libraries/ArbitrumMainnetDeployer.sol +++ b/script/libraries/ArbitrumMainnetDeployer.sol @@ -39,16 +39,16 @@ library ArbitrumMainnetDeployer { BaseDeployer.setupConfigHub(result.configHub, defaultHubParams()); // pairs - result.assetPairContracts = deployAllContractPairs(thisSender, result.configHub); + result.assetPairContracts = deployAllContractPairs(result.configHub); for (uint i = 0; i < result.assetPairContracts.length; i++) { BaseDeployer.setupContractPair(result.configHub, result.assetPairContracts[i]); } // ownership - BaseDeployer.nominateNewOwnerAll(finalOwner, result); + BaseDeployer.nominateNewHubOwner(finalOwner, result); } - function deployAllContractPairs(address initialOwner, ConfigHub configHub) + function deployAllContractPairs(ConfigHub configHub) internal returns (BaseDeployer.AssetPairContracts[] memory assetPairContracts) { @@ -83,12 +83,10 @@ library ArbitrumMainnetDeployer { ); // if any escrowNFT contracts will be reused for multiple pairs, they should be deployed first - EscrowSupplierNFT wethEscrow = - BaseDeployer.deployEscrowNFT(initialOwner, configHub, IERC20(WETH), "WETH"); + EscrowSupplierNFT wethEscrow = BaseDeployer.deployEscrowNFT(configHub, IERC20(WETH), "WETH"); // deploy pairs assetPairContracts[0] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WETH/USDC", @@ -104,7 +102,6 @@ library ArbitrumMainnetDeployer { ); assetPairContracts[1] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WETH/USDT", @@ -120,7 +117,6 @@ library ArbitrumMainnetDeployer { ); assetPairContracts[2] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WBTC/USDT", diff --git a/script/libraries/ArbitrumSepoliaDeployer.sol b/script/libraries/ArbitrumSepoliaDeployer.sol index 4f7186dd..70be12c8 100644 --- a/script/libraries/ArbitrumSepoliaDeployer.sol +++ b/script/libraries/ArbitrumSepoliaDeployer.sol @@ -40,13 +40,13 @@ library ArbitrumSepoliaDeployer { BaseDeployer.setupConfigHub(result.configHub, defaultHubParams()); // pairs - result.assetPairContracts = deployAllContractPairs(thisSender, result.configHub); + result.assetPairContracts = deployAllContractPairs(result.configHub); for (uint i = 0; i < result.assetPairContracts.length; i++) { BaseDeployer.setupContractPair(result.configHub, result.assetPairContracts[i]); } // ownership - BaseDeployer.nominateNewOwnerAll(finalOwner, result); + BaseDeployer.nominateNewHubOwner(finalOwner, result); } function deployMockOracleETHUSD() internal returns (BaseTakerOracle oracle) { @@ -90,7 +90,7 @@ library ArbitrumSepoliaDeployer { oracle = BaseDeployer.deployChainlinkOracle(tUSDC, Const.VIRTUAL_ASSET, feedUSDC_USD, sequencerFeed); } - function deployAllContractPairs(address initialOwner, ConfigHub configHub) + function deployAllContractPairs(ConfigHub configHub) internal returns (BaseDeployer.AssetPairContracts[] memory assetPairContracts) { @@ -104,7 +104,6 @@ library ArbitrumSepoliaDeployer { // if any escrowNFT contracts will be reused for multiple pairs, they should be deployed first assetPairContracts[0] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WETH/USDC", @@ -125,7 +124,6 @@ library ArbitrumSepoliaDeployer { ); assetPairContracts[1] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WBTC/USDC", diff --git a/script/libraries/BaseDeployer.sol b/script/libraries/BaseDeployer.sol index 41cd809e..38cf9518 100644 --- a/script/libraries/BaseDeployer.sol +++ b/script/libraries/BaseDeployer.sol @@ -66,14 +66,11 @@ library BaseDeployer { return new ConfigHub(initialOwner); } - function deployEscrowNFT( - address initialOwner, - ConfigHub configHub, - IERC20 underlying, - string memory underlyingSymbol - ) internal returns (EscrowSupplierNFT) { + function deployEscrowNFT(ConfigHub configHub, IERC20 underlying, string memory underlyingSymbol) + internal + returns (EscrowSupplierNFT) + { return new EscrowSupplierNFT( - initialOwner, configHub, underlying, string(abi.encodePacked("Escrow ", underlyingSymbol)), @@ -116,12 +113,11 @@ library BaseDeployer { ); } - function deployContractPair(address initialOwner, ConfigHub configHub, PairConfig memory pairConfig) + function deployContractPair(ConfigHub configHub, PairConfig memory pairConfig) internal returns (AssetPairContracts memory contracts) { CollarTakerNFT takerNFT = new CollarTakerNFT( - initialOwner, configHub, pairConfig.cashAsset, pairConfig.underlying, @@ -130,7 +126,6 @@ library BaseDeployer { string.concat("T", pairConfig.name) ); CollarProviderNFT providerNFT = new CollarProviderNFT( - initialOwner, configHub, pairConfig.cashAsset, pairConfig.underlying, @@ -139,12 +134,9 @@ library BaseDeployer { string.concat("P", pairConfig.name) ); LoansNFT loansContract = new LoansNFT( - initialOwner, - takerNFT, - string.concat("Loans ", pairConfig.name), - string.concat("L", pairConfig.name) + takerNFT, string.concat("Loans ", pairConfig.name), string.concat("L", pairConfig.name) ); - Rolls rollsContract = new Rolls(initialOwner, takerNFT); + Rolls rollsContract = new Rolls(takerNFT); SwapperUniV3 swapperUniV3 = new SwapperUniV3(pairConfig.swapRouter, pairConfig.swapFeeTier); // Use existing escrow if provided, otherwise create new EscrowSupplierNFT escrowNFT; @@ -152,10 +144,7 @@ library BaseDeployer { escrowNFT = EscrowSupplierNFT(pairConfig.existingEscrowNFT); } else { escrowNFT = deployEscrowNFT( - initialOwner, - configHub, - pairConfig.underlying, - IERC20Metadata(address(pairConfig.underlying)).symbol() + configHub, pairConfig.underlying, IERC20Metadata(address(pairConfig.underlying)).symbol() ); } contracts = AssetPairContracts({ @@ -194,38 +183,16 @@ library BaseDeployer { } // @dev this only nominates, and ownership must be accepted by the new owner - function nominateNewOwnerAll(address finalOwner, DeploymentResult memory result) internal { + function nominateNewHubOwner(address finalOwner, DeploymentResult memory result) internal { result.configHub.transferOwnership(finalOwner); - - for (uint i = 0; i < result.assetPairContracts.length; i++) { - nominateNewOwnerPair(finalOwner, result.assetPairContracts[i]); - } } - function nominateNewOwnerPair(address finalOwner, AssetPairContracts memory pair) internal { - pair.takerNFT.transferOwnership(finalOwner); - pair.providerNFT.transferOwnership(finalOwner); - pair.loansContract.transferOwnership(finalOwner); - pair.rollsContract.transferOwnership(finalOwner); - pair.escrowNFT.transferOwnership(finalOwner); - } - - function acceptOwnershipAsSender(address acceptingOwner, ConfigHub hub, AssetPairContracts[] memory pairs) - internal - { + function acceptOwnershipAsSender(address acceptingOwner, ConfigHub hub) internal { // we can't ensure sender is acceptingOwner by checking here because: // - if this is run in a script msg.sender will be the scripts's sender // - if it's run from a test it will be whoever is being pranked, or the test contract // - if ran from within a contract, will be the contract address // In any case, the acceptance will not work if sender is incorrect. hub.acceptOwnership(); - for (uint i = 0; i < pairs.length; i++) { - pairs[i].takerNFT.acceptOwnership(); - pairs[i].providerNFT.acceptOwnership(); - pairs[i].loansContract.acceptOwnership(); - pairs[i].rollsContract.acceptOwnership(); - // check because we may have already accepted previously (for another pair) - if (pairs[i].escrowNFT.owner() != acceptingOwner) pairs[i].escrowNFT.acceptOwnership(); - } } } diff --git a/script/libraries/OPBaseMainnetDeployer.sol b/script/libraries/OPBaseMainnetDeployer.sol index 52ad3332..3d04251e 100644 --- a/script/libraries/OPBaseMainnetDeployer.sol +++ b/script/libraries/OPBaseMainnetDeployer.sol @@ -37,16 +37,16 @@ library OPBaseMainnetDeployer { BaseDeployer.setupConfigHub(result.configHub, defaultHubParams()); // pairs - result.assetPairContracts = deployAllContractPairs(thisSender, result.configHub); + result.assetPairContracts = deployAllContractPairs(result.configHub); for (uint i = 0; i < result.assetPairContracts.length; i++) { BaseDeployer.setupContractPair(result.configHub, result.assetPairContracts[i]); } // ownership - BaseDeployer.nominateNewOwnerAll(finalOwner, result); + BaseDeployer.nominateNewHubOwner(finalOwner, result); } - function deployAllContractPairs(address initialOwner, ConfigHub configHub) + function deployAllContractPairs(ConfigHub configHub) internal returns (BaseDeployer.AssetPairContracts[] memory assetPairContracts) { @@ -76,7 +76,6 @@ library OPBaseMainnetDeployer { // deploy pairs assetPairContracts[0] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WETH/USDC", @@ -92,7 +91,6 @@ library OPBaseMainnetDeployer { ); assetPairContracts[1] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "cbBTC/USDC", diff --git a/script/libraries/OPBaseSepoliaDeployer.sol b/script/libraries/OPBaseSepoliaDeployer.sol index 6ae7ad1c..0d5d0e7a 100644 --- a/script/libraries/OPBaseSepoliaDeployer.sol +++ b/script/libraries/OPBaseSepoliaDeployer.sol @@ -40,13 +40,13 @@ library OPBaseSepoliaDeployer { BaseDeployer.setupConfigHub(result.configHub, defaultHubParams()); // pairs - result.assetPairContracts = deployAllContractPairs(thisSender, result.configHub); + result.assetPairContracts = deployAllContractPairs(result.configHub); for (uint i = 0; i < result.assetPairContracts.length; i++) { BaseDeployer.setupContractPair(result.configHub, result.assetPairContracts[i]); } // ownership - BaseDeployer.nominateNewOwnerAll(finalOwner, result); + BaseDeployer.nominateNewHubOwner(finalOwner, result); } function deployMockOracleETHUSD() internal returns (BaseTakerOracle oracle) { @@ -92,7 +92,7 @@ library OPBaseSepoliaDeployer { oracle = BaseDeployer.deployChainlinkOracle(tUSDC, Const.VIRTUAL_ASSET, feedUSDC_USD, sequencerFeed); } - function deployAllContractPairs(address initialOwner, ConfigHub configHub) + function deployAllContractPairs(ConfigHub configHub) internal returns (BaseDeployer.AssetPairContracts[] memory assetPairContracts) { @@ -106,7 +106,6 @@ library OPBaseSepoliaDeployer { // if any escrowNFT contracts will be reused for multiple pairs, they should be deployed first assetPairContracts[0] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WETH/USDC", @@ -127,7 +126,6 @@ library OPBaseSepoliaDeployer { ); assetPairContracts[1] = BaseDeployer.deployContractPair( - initialOwner, configHub, BaseDeployer.PairConfig({ name: "WBTC/USDC", diff --git a/src/CollarProviderNFT.sol b/src/CollarProviderNFT.sol index 957daf7a..e93854c6 100644 --- a/src/CollarProviderNFT.sol +++ b/src/CollarProviderNFT.sol @@ -48,7 +48,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { uint public constant MAX_PUT_STRIKE_BIPS = BIPS_BASE - 1; // 1 less than 1x uint public constant MAX_PROTOCOL_FEE_BIPS = BIPS_BASE / 100; // 1% - string public constant VERSION = "0.2.0"; + string public constant VERSION = "0.3.0"; // ----- IMMUTABLES ----- // IERC20 public immutable cashAsset; @@ -65,14 +65,13 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { mapping(uint positionId => ProviderPositionStored) internal positions; constructor( - address initialOwner, ConfigHub _configHub, IERC20 _cashAsset, IERC20 _underlying, address _taker, string memory _name, string memory _symbol - ) BaseNFT(initialOwner, _name, _symbol, _configHub, address(_cashAsset)) { + ) BaseNFT(_name, _symbol, _configHub, address(_cashAsset)) { cashAsset = _cashAsset; underlying = _underlying; taker = _taker; @@ -197,7 +196,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { uint putStrikePercent, uint duration, uint minLocked - ) external whenNotPaused returns (uint offerId) { + ) external returns (uint offerId) { // sanity checks require(callStrikePercent >= MIN_CALL_STRIKE_BIPS, "provider: strike percent too low"); require(callStrikePercent <= MAX_CALL_STRIKE_BIPS, "provider: strike percent too high"); @@ -230,7 +229,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { * can be a low likelihood concern on a network that exposes a public mempool. * Avoid it by not granting excessive ERC-20 approvals. */ - function updateOfferAmount(uint offerId, uint newAmount) external whenNotPaused { + function updateOfferAmount(uint offerId, uint newAmount) external { LiquidityOfferStored storage offer = liquidityOffers[offerId]; require(msg.sender == offer.provider, "provider: not offer provider"); @@ -267,7 +266,6 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { /// @return positionId The ID of the newly created position (NFT token ID) function mintFromOffer(uint offerId, uint providerLocked, uint takerId) external - whenNotPaused onlyTaker returns (uint positionId) { @@ -335,7 +333,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { /// of the NFT abruptly (only prevent settlement at future price). /// @param positionId The ID of the position to settle (NFT token ID) /// @param cashDelta The change in position value (positive or negative) - function settlePosition(uint positionId, int cashDelta) external whenNotPaused onlyTaker { + function settlePosition(uint positionId, int cashDelta) external onlyTaker { ProviderPositionStored storage position = positions[positionId]; require(position.expiration != 0, "provider: position does not exist"); @@ -371,7 +369,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { /// to settlement, during cancellation the taker's caller MUST be the NFT owner (is the provider), /// so is assumed to specify the withdrawal correctly for their funds. /// @param positionId The ID of the position to cancel (NFT token ID) - function cancelAndWithdraw(uint positionId) external whenNotPaused onlyTaker returns (uint withdrawal) { + function cancelAndWithdraw(uint positionId) external onlyTaker returns (uint withdrawal) { ProviderPositionStored storage position = positions[positionId]; require(position.expiration != 0, "provider: position does not exist"); require(!position.settled, "provider: already settled"); @@ -405,7 +403,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { /// @notice Withdraws funds from a settled position. Can only be called for a settled position /// (and not a cancelled one), and checks the ownership of the NFT. Burns the NFT. /// @param positionId The ID of the settled position to withdraw from (NFT token ID). - function withdrawFromSettled(uint positionId) external whenNotPaused returns (uint withdrawal) { + function withdrawFromSettled(uint positionId) external returns (uint withdrawal) { // Note: _isAuthorized not used here to reduce surface area / KISS. Can be used if needed, // since an approved account can pull and withdraw. require(msg.sender == ownerOf(positionId), "provider: not position owner"); diff --git a/src/CollarTakerNFT.sol b/src/CollarTakerNFT.sol index 4fdeed6e..b119d84c 100644 --- a/src/CollarTakerNFT.sol +++ b/src/CollarTakerNFT.sol @@ -46,7 +46,7 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { uint internal constant BIPS_BASE = 10_000; - string public constant VERSION = "0.2.0"; + string public constant VERSION = "0.3.0"; // ----- IMMUTABLES ----- // IERC20 public immutable cashAsset; @@ -58,14 +58,13 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { mapping(uint positionId => TakerPositionStored) internal positions; constructor( - address initialOwner, ConfigHub _configHub, IERC20 _cashAsset, IERC20 _underlying, ITakerOracle _oracle, string memory _name, string memory _symbol - ) BaseNFT(initialOwner, _name, _symbol, _configHub, address(_cashAsset)) { + ) BaseNFT(_name, _symbol, _configHub, address(_cashAsset)) { cashAsset = _cashAsset; underlying = _underlying; _setOracle(_oracle); @@ -168,7 +167,6 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { */ function openPairedPosition(uint takerLocked, CollarProviderNFT providerNFT, uint offerId) external - whenNotPaused nonReentrant returns (uint takerId, uint providerId) { @@ -235,7 +233,7 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { * one side is not (e.g., due to being at max loss). For this reason a keeper should be run to * prevent users with gains from not settling their positions on time. */ - function settlePairedPosition(uint takerId) external whenNotPaused nonReentrant { + function settlePairedPosition(uint takerId) external nonReentrant { // @dev this checks position exists TakerPosition memory position = getPosition(takerId); @@ -270,12 +268,7 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { /// @notice Withdraws funds from a settled position. Burns the NFT. /// @param takerId The ID of the settled position to withdraw from (NFT token ID). /// @return withdrawal The amount of cash asset withdrawn - function withdrawFromSettled(uint takerId) - external - whenNotPaused - nonReentrant - returns (uint withdrawal) - { + function withdrawFromSettled(uint takerId) external nonReentrant returns (uint withdrawal) { require(msg.sender == ownerOf(takerId), "taker: not position owner"); TakerPosition memory position = getPosition(takerId); @@ -298,12 +291,7 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { * @param takerId The ID of the taker position to cancel * @return withdrawal The amount of funds withdrawn from both positions together */ - function cancelPairedPosition(uint takerId) - external - whenNotPaused - nonReentrant - returns (uint withdrawal) - { + function cancelPairedPosition(uint takerId) external nonReentrant returns (uint withdrawal) { TakerPosition memory position = getPosition(takerId); (CollarProviderNFT providerNFT, uint providerId) = (position.providerNFT, position.providerId); @@ -338,11 +326,11 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { ); } - // ----- Owner Mutative ----- // + // ----- admin Mutative ----- // /// @notice Sets the price oracle used by the contract /// @param _oracle The new price oracle to use - function setOracle(ITakerOracle _oracle) external onlyOwner { + function setOracle(ITakerOracle _oracle) external onlyConfigHubOwner { _setOracle(_oracle); } diff --git a/src/EscrowSupplierNFT.sol b/src/EscrowSupplierNFT.sol index de83de9f..fc3adcb2 100644 --- a/src/EscrowSupplierNFT.sol +++ b/src/EscrowSupplierNFT.sol @@ -52,7 +52,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { // @notice max percentage of refunded interest fee, prevents free cancellation issues uint public constant MAX_FEE_REFUND_BIPS = 9500; // 95% - string public constant VERSION = "0.2.0"; + string public constant VERSION = "0.3.0"; // ----- IMMUTABLES ----- // IERC20 public immutable asset; // corresponds to Loans' underlying @@ -68,13 +68,9 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { mapping(uint escrowId => EscrowStored) internal escrows; - constructor( - address initialOwner, - ConfigHub _configHub, - IERC20 _asset, - string memory _name, - string memory _symbol - ) BaseNFT(initialOwner, _name, _symbol, _configHub, address(_asset)) { + constructor(ConfigHub _configHub, IERC20 _asset, string memory _name, string memory _symbol) + BaseNFT(_name, _symbol, _configHub, address(_asset)) + { asset = _asset; } @@ -195,7 +191,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { uint gracePeriod, uint lateFeeAPR, uint minEscrow - ) external whenNotPaused returns (uint offerId) { + ) external returns (uint offerId) { // sanity checks require(interestAPR <= MAX_INTEREST_APR_BIPS, "escrow: interest APR too high"); require(lateFeeAPR <= MAX_LATE_FEE_APR_BIPS, "escrow: late fee APR too high"); @@ -228,7 +224,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { * can be a low likelihood concern on a network that exposes a public mempool. * Avoid it by not granting excessive ERC-20 approvals. */ - function updateOfferAmount(uint offerId, uint newAmount) external whenNotPaused { + function updateOfferAmount(uint offerId, uint newAmount) external { OfferStored storage offer = offers[offerId]; require(msg.sender == offer.supplier, "escrow: not offer supplier"); @@ -267,7 +263,6 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { */ function startEscrow(uint offerId, uint escrowed, uint fees, uint loanId) external - whenNotPaused returns (uint escrowId) { // @dev msg.sender auth is checked vs. loansCanOpen in _startEscrow @@ -292,7 +287,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { * but doesn't have to be. Any under or overpayment, or fee refunds will effect toLoans. * @return toLoans Amount to be returned to loans (refund deducing shortfalls) */ - function endEscrow(uint escrowId, uint repaid) external whenNotPaused returns (uint toLoans) { + function endEscrow(uint escrowId, uint repaid) external returns (uint toLoans) { // @dev msg.sender auth is checked vs. stored loans in _endEscrow toLoans = _endEscrow(escrowId, getEscrow(escrowId), repaid); @@ -322,7 +317,6 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { */ function switchEscrow(uint releaseEscrowId, uint offerId, uint newFees, uint newLoanId) external - whenNotPaused returns (uint newEscrowId, uint feesRefund) { Escrow memory previousEscrow = getEscrow(releaseEscrowId); @@ -362,7 +356,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { /// @notice Withdraws funds from a released escrow. Burns the NFT. /// @param escrowId The ID of the escrow to withdraw from - function withdrawReleased(uint escrowId) external whenNotPaused { + function withdrawReleased(uint escrowId) external { require(msg.sender == ownerOf(escrowId), "escrow: not escrow owner"); // will revert for burned Escrow memory escrow = getEscrow(escrowId); @@ -388,7 +382,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { * In either case, escrow owner withdraws full principal, interest, and late fees. * @param escrowId The ID of the escrow to seize */ - function seizeEscrow(uint escrowId) external whenNotPaused { + function seizeEscrow(uint escrowId) external { require(msg.sender == ownerOf(escrowId), "escrow: not escrow owner"); // will revert for burned Escrow memory escrow = getEscrow(escrowId); @@ -413,7 +407,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { // ----- admin ----- // /// @notice Sets whether a Loans contract is allowed to interact with this contract - function setLoansCanOpen(address loans, bool allowed) external onlyOwner { + function setLoansCanOpen(address loans, bool allowed) external onlyConfigHubOwner { // @dev no checks for Loans interface since calls are only from Loans to this contract loansCanOpen[loans] = allowed; emit LoansCanOpenSet(loans, allowed); diff --git a/src/LoansNFT.sol b/src/LoansNFT.sol index de6506d2..595e6b40 100644 --- a/src/LoansNFT.sol +++ b/src/LoansNFT.sol @@ -48,7 +48,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { uint internal constant BIPS_BASE = 10_000; - string public constant VERSION = "0.2.0"; + string public constant VERSION = "0.3.0"; uint public constant MAX_SWAP_PRICE_DEVIATION_BIPS = 1000; // 10%, allows 10% self-sandwich slippage // ----- IMMUTABLES ----- // @@ -73,8 +73,8 @@ contract LoansNFT is ILoansNFT, BaseNFT { // a convenience view to allow querying for a swapper onchain / FE without subgraph address public defaultSwapper; - constructor(address initialOwner, CollarTakerNFT _takerNFT, string memory _name, string memory _symbol) - BaseNFT(initialOwner, _name, _symbol, _takerNFT.configHub(), address(_takerNFT)) + constructor(CollarTakerNFT _takerNFT, string memory _name, string memory _symbol) + BaseNFT(_name, _symbol, _takerNFT.configHub(), address(_takerNFT)) { takerNFT = _takerNFT; cashAsset = _takerNFT.cashAsset(); @@ -138,7 +138,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { uint minLoanAmount, SwapParams calldata swapParams, ProviderOffer calldata providerOffer - ) external whenNotPaused returns (uint loanId, uint providerId, uint loanAmount) { + ) external returns (uint loanId, uint providerId, uint loanAmount) { EscrowOffer memory noEscrow = EscrowOffer(EscrowSupplierNFT(address(0)), 0); return _openLoan(underlyingAmount, minLoanAmount, swapParams, providerOffer, false, noEscrow, 0); } @@ -171,7 +171,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { ProviderOffer calldata providerOffer, EscrowOffer calldata escrowOffer, uint escrowFees - ) external whenNotPaused returns (uint loanId, uint providerId, uint loanAmount) { + ) external returns (uint loanId, uint providerId, uint loanAmount) { return _openLoan( underlyingAmount, minLoanAmount, swapParams, providerOffer, true, escrowOffer, escrowFees ); @@ -209,11 +209,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { * - any extraData the swapper needs to use * @return underlyingOut The actual amount of underlying asset returned to the user */ - function closeLoan(uint loanId, SwapParams calldata swapParams) - external - whenNotPaused - returns (uint underlyingOut) - { + function closeLoan(uint loanId, SwapParams calldata swapParams) external returns (uint underlyingOut) { // @dev cache the borrower now, since _closeLoan will burn the NFT, so ownerOf will revert // Borrower is the NFT owner, since msg.sender can be a keeper. // If called by keeper, the borrower must trust it because: @@ -279,7 +275,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { int minToUser, uint newEscrowOfferId, uint newEscrowFee - ) external whenNotPaused onlyNFTOwner(loanId) returns (uint newLoanId, uint newLoanAmount, int toUser) { + ) external onlyNFTOwner(loanId) returns (uint newLoanId, uint newLoanAmount, int toUser) { // check opening loans is still allowed (not in exit-only mode) require(configHub.canOpenPair(underlying, cashAsset, address(this)), "loans: unsupported loans"); @@ -339,7 +335,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { * escrow fee refunds to the caller. * @param loanId The ID representing the loan to unwrap and cancel */ - function unwrapAndCancelLoan(uint loanId) external whenNotPaused onlyNFTOwner(loanId) { + function unwrapAndCancelLoan(uint loanId) external onlyNFTOwner(loanId) { // release escrow if needed with refunds (interest fee) to sender _conditionalCheckAndCancelEscrow(loanId); @@ -360,7 +356,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { * @param loanId specific loanId which the user approves the keeper to close * @param enabled True to allow the keeper, false to disallow */ - function setKeeperApproved(uint loanId, bool enabled) external whenNotPaused { + function setKeeperApproved(uint loanId, bool enabled) external { keeperApprovedFor[msg.sender][loanId] = enabled; emit ClosingKeeperApproved(msg.sender, loanId, enabled); } @@ -370,8 +366,8 @@ contract LoansNFT is ILoansNFT, BaseNFT { /// @notice Sets the address of the single allowed closing keeper. Alternative decentralized keeper /// arrangements can be added via composability, e.g., a contract that would hold the NFTs (thus /// will be able to perform actions) and allow incentivised keepers to call it. - /// @dev only owner - function setKeeper(address keeper) external onlyOwner { + /// @dev only configHub owner + function setKeeper(address keeper) external onlyConfigHubOwner { emit ClosingKeeperUpdated(closingKeeper, keeper); closingKeeper = keeper; } @@ -380,8 +376,8 @@ contract LoansNFT is ILoansNFT, BaseNFT { /// When no swapper is allowed, opening and closing loans will not be possible, only cancelling. /// The default swapper is a convenience view, and it's best to keep it up to date /// and to make sure the default one is allowed. - /// @dev only owner - function setSwapperAllowed(address swapper, bool allow, bool setDefault) external onlyOwner { + /// @dev only configHub owner + function setSwapperAllowed(address swapper, bool allow, bool setDefault) external onlyConfigHubOwner { if (allow) require(bytes(ISwapper(swapper).VERSION()).length > 0, "loans: invalid swapper"); allow ? allowedSwappers.add(swapper) : allowedSwappers.remove(swapper); diff --git a/src/Rolls.sol b/src/Rolls.sol index f67651e9..bedf2518 100644 --- a/src/Rolls.sol +++ b/src/Rolls.sol @@ -7,7 +7,6 @@ import { SignedMath } from "@openzeppelin/contracts/utils/math/SignedMath.sol"; import { CollarTakerNFT, ICollarTakerNFT } from "./CollarTakerNFT.sol"; import { CollarProviderNFT, ICollarProviderNFT } from "./CollarProviderNFT.sol"; -import { BaseManaged } from "./base/BaseManaged.sol"; import { IRolls } from "./interfaces/IRolls.sol"; /** @@ -44,13 +43,13 @@ import { IRolls } from "./interfaces/IRolls.sol"; * - ConfigHub: If this contract is used through Loans, set setCanOpenPair() to authorize this * contract for its asset pair */ -contract Rolls is IRolls, BaseManaged { +contract Rolls is IRolls { using SafeERC20 for IERC20; using SafeCast for uint; uint internal constant BIPS_BASE = 10_000; - string public constant VERSION = "0.2.0"; + string public constant VERSION = "0.3.0"; // ----- IMMUTABLES ----- // CollarTakerNFT public immutable takerNFT; @@ -62,18 +61,10 @@ contract Rolls is IRolls, BaseManaged { mapping(uint rollId => RollOfferStored) internal rollOffers; - /// @dev Rolls needs BaseManaged for pausing since is approved by users, and holds NFTs. - /// Does not need `canOpen` auth here because its auth is checked in Loans, or requires no auth + /// @dev Rolls does not need `canOpen` auth because its auth is checked in Loans, or requires no auth /// if used directly. /// Has no long-lived functionality so doesn't need a close-only migration mode. - /// @dev unrescuableAsset is not set, which means that providerNFTs held by this contract for active - /// offers can be rescued by the owner via `rescueTokens`. In a future implementation this can be set - /// to a specific providerNFT if needed (limiting the Rolls contract to a single provider NFT). Currently, - /// because roll offers are short lived and optional, the added complexity is not worth it wrt - /// to the risk mitigated by it. - constructor(address initialOwner, CollarTakerNFT _takerNFT) - BaseManaged(initialOwner, _takerNFT.configHub(), address(0)) - { + constructor(CollarTakerNFT _takerNFT) { takerNFT = _takerNFT; cashAsset = _takerNFT.cashAsset(); } @@ -184,7 +175,7 @@ contract Rolls is IRolls, BaseManaged { uint maxPrice, int minToProvider, uint deadline - ) external whenNotPaused returns (uint rollId) { + ) external returns (uint rollId) { // taker position is valid ICollarTakerNFT.TakerPosition memory takerPos = takerNFT.getPosition(takerId); require(takerPos.expiration != 0, "rolls: taker position does not exist"); @@ -232,7 +223,7 @@ contract Rolls is IRolls, BaseManaged { * The risk of update is different here from providerNFT.updateOfferAmount because * the most an update can cause there is a revert of taking the offer. */ - function cancelOffer(uint rollId) external whenNotPaused { + function cancelOffer(uint rollId) external { RollOffer memory offer = getRollOffer(rollId); require(msg.sender == offer.provider, "rolls: not offer provider"); require(offer.active, "rolls: offer not active"); @@ -258,7 +249,6 @@ contract Rolls is IRolls, BaseManaged { */ function executeRoll(uint rollId, int minToTaker) external - whenNotPaused returns (uint newTakerId, uint newProviderId, int toTaker, int toProvider) { RollOffer memory offer = getRollOffer(rollId); diff --git a/src/base/BaseManaged.sol b/src/base/BaseManaged.sol index e8182e9c..99364c3c 100644 --- a/src/base/BaseManaged.sol +++ b/src/base/BaseManaged.sol @@ -2,100 +2,88 @@ pragma solidity 0.8.22; import { SafeERC20, IERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import { Ownable2Step, Ownable } from "@openzeppelin/contracts/access/Ownable2Step.sol"; -import { Pausable } from "@openzeppelin/contracts/utils/Pausable.sol"; import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import { ConfigHub } from "../ConfigHub.sol"; /** * @title BaseManaged - * @notice Base contract for managed contracts in the Collar Protocol - * @dev This contract provides common functionality for contracts that are owned and managed - * via the Collar Protocol's ConfigHub. It includes 2 step ownership, pause/unpause functionality, - * pause guardian pausing, and a function to rescue stuck tokens. - * @dev all inheriting contracts that hold funds should be owned by a secure multi-sig due - * the sensitive onlyOwner methods. + * @notice Base contract for managed contracts in the protocol + * @dev This contract provides common functionality for contracts that are managed + * via the a common ConfigHub. It includes a onlyConfigHubOwner that can control access to + * configuration value setters in inheriting contracts, and a function to rescue stuck tokens. + * @dev ConfigHub should be owned by a secure multi-sig due the sensitive onlyConfigHubOwner methods. */ -abstract contract BaseManaged is Ownable2Step, Pausable { +abstract contract BaseManaged { // ----- State ----- // ConfigHub public configHub; /** - * @notice a token address (ERC-20 or ERC-721) that the owner can NOT rescue via `rescueTokens`. + * @notice a token address (ERC-20 or ERC-721) that configHub's owner can NOT rescue via `rescueTokens`. * This should be the main asset that the contract holds at rest on behalf of its users. * If this asset is sent to the contract by mistake, there is no way to rescue it. * This exclusion reduces the centralisation risk, and reduces the impact, incentive, and - * thus also the likelihood of compromizing the owner. + * thus also the likelihood of compromizing the configHub's owner. * @dev this is a single address because currently only a single asset is held by each contract * at rest (except for Rolls). * Other assets, that may be held by the contract in-transit (during a transaction), can be rescued. - * If further reduction of centralization risk is required, this should become a Set. + * If more assets need to be unrescuable, this should become a Set. */ address public immutable unrescuableAsset; // ----- Events ----- // event ConfigHubUpdated(ConfigHub previousConfigHub, ConfigHub newConfigHub); - event PausedByGuardian(address guardian); event TokensRescued(address tokenContract, uint amountOrId); - constructor(address _initialOwner, ConfigHub _configHub, address _unrescuableAsset) - Ownable(_initialOwner) - { + constructor(ConfigHub _configHub, address _unrescuableAsset) { _setConfigHub(_configHub); unrescuableAsset = _unrescuableAsset; } - // ----- MUTATIVE ----- // - - // ----- Non-owner ----- // - - // @notice Pause method called from a guardian authorized by the ConfigHub in case of emergency - // Reverts if sender is not guardian, or if owner is revoked (since unpausing would be impossible) - function pauseByGuardian() external { - require(configHub.isPauseGuardian(msg.sender), "not guardian"); - // if owner is renounced, no one will be able to call unpause. - // Using Ownable2Step ensures the owner can only be renounced to address(0). - require(owner() != address(0), "owner renounced"); + // ----- Modifiers ----- // - _pause(); // @dev also emits Paused - emit PausedByGuardian(msg.sender); + modifier onlyConfigHubOwner() { + require(msg.sender == configHub.owner(), "BaseManaged: not configHub owner"); + _; } - // ----- owner ----- // + // ----- Views ----- // - /// @notice Pauses the contract when called by the contract owner in case of emergency. - function pause() external onlyOwner { - _pause(); + function configHubOwner() public view returns (address) { + return configHub.owner(); } - /// @notice Unpauses the contract when called by the contract owner - function unpause() external onlyOwner { - _unpause(); - } + // ----- MUTATIVE ----- // + + // ----- onlyConfigHubOwner ----- // - /// @notice Updates the address of the ConfigHub contract when called by the contract owner - function setConfigHub(ConfigHub _newConfigHub) external onlyOwner { + /// @notice Updates the address of the ConfigHub contract when called by the current configHub's owner. + /// Only allows switching to a new configHub whose owner is the same as current configHub's owner. + /// @dev To renounce access to specific contracts use setConfigHub to set a new configHub controlled + /// by the same owner, and then renounce its ownership. + function setConfigHub(ConfigHub _newConfigHub) external onlyConfigHubOwner { + // check owner view exists and returns same address to prevent accidentally getting locked out. + require(_newConfigHub.owner() == configHubOwner(), "BaseManaged: new configHub owner mismatch"); _setConfigHub(_newConfigHub); } /** - * @notice Sends an amount of an ERC-20, or an ID of any ERC-721, to owner's address. + * @notice Sends an amount of an ERC-20, or an ID of any ERC-721, to configHub owner's address. * Reverts if the `token` matches the `unrescuableAsset`. * To be used to rescue stuck assets that were sent to the contract by mistake. - * @dev Only callable by the contract owner. + * @dev Only callable by the configHub's owner. * @param token The address of the token contract * @param amountOrId The amount of tokens to rescue (or token ID for NFTs) * @param isNFT Whether the token is an NFT (true) or an ERC-20 (false) */ - function rescueTokens(address token, uint amountOrId, bool isNFT) external onlyOwner { + function rescueTokens(address token, uint amountOrId, bool isNFT) external onlyConfigHubOwner { require(token != unrescuableAsset, "unrescuable asset"); if (isNFT) { - IERC721(token).transferFrom(address(this), owner(), amountOrId); + IERC721(token).transferFrom(address(this), configHubOwner(), amountOrId); } else { // for ERC-20 must use transfer, since most implementation won't allow transferFrom // without approve (even from owner) - SafeERC20.safeTransfer(IERC20(token), owner(), amountOrId); + SafeERC20.safeTransfer(IERC20(token), configHubOwner(), amountOrId); } emit TokensRescued(token, amountOrId); } diff --git a/src/base/BaseNFT.sol b/src/base/BaseNFT.sol index 3bb140b4..370f6823 100644 --- a/src/base/BaseNFT.sol +++ b/src/base/BaseNFT.sol @@ -7,7 +7,7 @@ import { BaseManaged, ConfigHub } from "./BaseManaged.sol"; /** * @title BaseNFT - * @notice Base contract for NFTs in the Collar Protocol. It provides the admin functionality + * @notice Base contract for NFTs in the protocol. It provides the admin functionality * from BaseManaged, and the NFT metadata base URI. */ abstract contract BaseNFT is BaseManaged, ERC721 { @@ -16,13 +16,10 @@ abstract contract BaseNFT is BaseManaged, ERC721 { // ----- State ----- // uint internal nextTokenId = 1; // NFT token ID, starts from 1 so that 0 ID is not used - constructor( - address _initialOwner, - string memory _name, - string memory _symbol, - ConfigHub _configHub, - address _unrescuableAsset - ) BaseManaged(_initialOwner, _configHub, _unrescuableAsset) ERC721(_name, _symbol) { } + constructor(string memory _name, string memory _symbol, ConfigHub _configHub, address _unrescuableAsset) + BaseManaged(_configHub, _unrescuableAsset) + ERC721(_name, _symbol) + { } // ----- INTERNAL ----- // diff --git a/test/integration/full-protocol/AssetDeployForkTest.t.sol b/test/integration/full-protocol/AssetDeployForkTest.t.sol index 1064a9ee..60084417 100644 --- a/test/integration/full-protocol/AssetDeployForkTest.t.sol +++ b/test/integration/full-protocol/AssetDeployForkTest.t.sol @@ -89,7 +89,6 @@ contract AssetDeployForkTest is BaseAssetPairForkTest { // deploy and configure new pair pairs = new BaseDeployer.AssetPairContracts[](1); pairs[0] = BaseDeployer.deployContractPair( - owner, hub, BaseDeployer.PairConfig({ name: "tWBTC-tUSDC", diff --git a/test/integration/full-protocol/BaseAssetPairForkTest.sol b/test/integration/full-protocol/BaseAssetPairForkTest.sol index c9587786..2fb497ca 100644 --- a/test/integration/full-protocol/BaseAssetPairForkTest.sol +++ b/test/integration/full-protocol/BaseAssetPairForkTest.sol @@ -351,6 +351,7 @@ abstract contract BaseAssetPairForkTest is Test { function test_validatePairDeployment() public view { // configHub + assertEq(configHub.VERSION(), "0.2.0"); assertEq(configHub.owner(), owner); assertEq(uint(configHub.protocolFeeAPR()), protocolFeeAPR); assertEq(configHub.feeRecipient(), protocolFeeRecipient); @@ -362,27 +363,29 @@ abstract contract BaseAssetPairForkTest is Test { assertEq(pair.oracle.description(), oracleDescription); // taker - assertEq(address(pair.takerNFT.owner()), owner); + assertEq(pair.takerNFT.VERSION(), "0.3.0"); + assertEq(address(pair.takerNFT.configHubOwner()), owner); assertEq(address(pair.takerNFT.configHub()), address(configHub)); assertEq(address(pair.takerNFT.underlying()), address(pair.underlying)); assertEq(address(pair.takerNFT.cashAsset()), address(pair.cashAsset)); assertEq(address(pair.takerNFT.oracle()), address(pair.oracle)); // provider - assertEq(address(pair.providerNFT.owner()), owner); + assertEq(pair.providerNFT.VERSION(), "0.3.0"); + assertEq(address(pair.providerNFT.configHubOwner()), owner); assertEq(address(pair.providerNFT.configHub()), address(configHub)); assertEq(address(pair.providerNFT.underlying()), address(pair.underlying)); assertEq(address(pair.providerNFT.cashAsset()), address(pair.cashAsset)); assertEq(address(pair.providerNFT.taker()), address(pair.takerNFT)); // rolls - assertEq(address(pair.rollsContract.owner()), owner); - assertEq(address(pair.rollsContract.configHub()), address(configHub)); + assertEq(pair.rollsContract.VERSION(), "0.3.0"); assertEq(address(pair.rollsContract.takerNFT()), address(pair.takerNFT)); assertEq(address(pair.rollsContract.cashAsset()), address(pair.cashAsset)); // loans - assertEq(address(pair.loansContract.owner()), owner); + assertEq(pair.loansContract.VERSION(), "0.3.0"); + assertEq(address(pair.loansContract.configHubOwner()), owner); assertEq(address(pair.loansContract.configHub()), address(configHub)); assertEq(address(pair.loansContract.takerNFT()), address(pair.takerNFT)); assertEq(address(pair.loansContract.underlying()), address(pair.underlying)); @@ -396,7 +399,8 @@ abstract contract BaseAssetPairForkTest is Test { assertEq(pair.loansContract.closingKeeper(), address(0)); // escrow - assertEq(address(pair.escrowNFT.owner()), owner); + assertEq(pair.escrowNFT.VERSION(), "0.3.0"); + assertEq(address(pair.escrowNFT.configHubOwner()), owner); assertEq(address(pair.escrowNFT.configHub()), address(configHub)); assertTrue(pair.escrowNFT.loansCanOpen(address(pair.loansContract))); assertEq(address(pair.escrowNFT.asset()), address(pair.underlying)); diff --git a/test/unit/BaseAssetPairTestSetup.sol b/test/unit/BaseAssetPairTestSetup.sol index 39e06151..02fd824f 100644 --- a/test/unit/BaseAssetPairTestSetup.sol +++ b/test/unit/BaseAssetPairTestSetup.sol @@ -75,22 +75,21 @@ contract BaseAssetPairTestSetup is Test { // taker checks oracle price on construction updatePrice(); - takerNFT = new CollarTakerNFT( - owner, configHub, cashAsset, underlying, chainlinkOracle, "CollarTakerNFT", "TKRNFT" - ); + takerNFT = + new CollarTakerNFT(configHub, cashAsset, underlying, chainlinkOracle, "CollarTakerNFT", "TKRNFT"); providerNFT = new CollarProviderNFT( - owner, configHub, cashAsset, underlying, address(takerNFT), "ProviderNFT", "PRVNFT" + configHub, cashAsset, underlying, address(takerNFT), "ProviderNFT", "PRVNFT" ); // this is to avoid having the paired IDs being equal providerNFT2 = new CollarProviderNFT( - owner, configHub, cashAsset, underlying, address(takerNFT), "ProviderNFT-2", "PRVNFT-2" + configHub, cashAsset, underlying, address(takerNFT), "ProviderNFT-2", "PRVNFT-2" ); vm.label(address(chainlinkOracle), "MockChainlinkOracle"); vm.label(address(takerNFT), "CollarTakerNFT"); vm.label(address(providerNFT), "CollarProviderNFT"); // asset pair periphery - rolls = new Rolls(owner, takerNFT); + rolls = new Rolls(takerNFT); vm.label(address(rolls), "Rolls"); } diff --git a/test/unit/BaseManaged.all.t.sol b/test/unit/BaseManaged.all.t.sol index 5e42368f..987edb03 100644 --- a/test/unit/BaseManaged.all.t.sol +++ b/test/unit/BaseManaged.all.t.sol @@ -4,8 +4,6 @@ pragma solidity 0.8.22; import "forge-std/Test.sol"; -import { Pausable } from "@openzeppelin/contracts/utils/Pausable.sol"; -import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import { TestERC20 } from "../utils/TestERC20.sol"; @@ -17,7 +15,6 @@ import { CollarTakerNFT } from "../../src/CollarTakerNFT.sol"; import { CollarProviderNFT } from "../../src/CollarProviderNFT.sol"; import { EscrowSupplierNFT } from "../../src/EscrowSupplierNFT.sol"; import { LoansNFT } from "../../src/LoansNFT.sol"; -import { Rolls } from "../../src/Rolls.sol"; import { ChainlinkOracle } from "../../src/ChainlinkOracle.sol"; contract TestERC721 is ERC721 { @@ -39,7 +36,6 @@ abstract contract BaseManagedTestBase is Test { address owner = makeAddr("owner"); address user1 = makeAddr("user1"); - address guardian = makeAddr("guardian"); function setUp() public virtual { erc20Rescuable = new TestERC20("TestERC20", "TestERC20", 18); @@ -57,54 +53,11 @@ abstract contract BaseManagedTestBase is Test { function test_constructor() public { setupTestedContract(); - assertEq(testedContract.owner(), owner); + assertEq(testedContract.configHubOwner(), owner); assertEq(address(testedContract.configHub()), address(configHub)); assertEq(address(testedContract.unrescuableAsset()), unrescuable); } - function test_pauseByGuardian() public { - vm.prank(owner); - configHub.setPauseGuardian(guardian, true); - - vm.prank(guardian); - vm.expectEmit(address(testedContract)); - emit Pausable.Paused(guardian); - vm.expectEmit(address(testedContract)); - emit BaseManaged.PausedByGuardian(guardian); - testedContract.pauseByGuardian(); - - assertTrue(testedContract.paused()); - - // unset - vm.startPrank(owner); - testedContract.unpause(); - configHub.setPauseGuardian(guardian, false); - - vm.startPrank(guardian); - vm.expectRevert("not guardian"); - testedContract.pauseByGuardian(); - } - - function test_pause() public { - vm.prank(owner); - vm.expectEmit(address(testedContract)); - emit Pausable.Paused(owner); - testedContract.pause(); - - assertTrue(testedContract.paused()); - } - - function test_unpause() public { - vm.startPrank(owner); - testedContract.pause(); - vm.expectEmit(address(testedContract)); - emit Pausable.Unpaused(owner); - testedContract.unpause(); - vm.stopPrank(); - - assertFalse(testedContract.paused()); - } - function test_setConfigHub() public { ConfigHub newConfigHub = new ConfigHub(owner); @@ -155,62 +108,31 @@ abstract contract BaseManagedTestBase is Test { vm.expectRevert(new bytes(0)); testedContract.setConfigHub(badHub); - badHub = ConfigHub(address(new BadConfigHub2())); + badHub = ConfigHub(address(new BadConfigHub2(owner))); vm.expectRevert("invalid ConfigHub"); testedContract.setConfigHub(badHub); } - function test_onlyOwnerMethods() public { - startHoax(user1); - bytes4 selector = Ownable.OwnableUnauthorizedAccount.selector; - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - testedContract.pause(); - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - testedContract.unpause(); - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - testedContract.setConfigHub(configHub); - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - testedContract.rescueTokens(address(0), 0, true); - } + function test_revert_setConfigHub_invalidConfigHubOwner() public { + vm.startPrank(owner); - function test_revert_pauseByGuardian_notGuardian() public { - vm.prank(user1); - vm.expectRevert("not guardian"); - testedContract.pauseByGuardian(); + ConfigHub hubDifferentOwner = new ConfigHub(user1); + vm.expectRevert("BaseManaged: new configHub owner mismatch"); + testedContract.setConfigHub(hubDifferentOwner); - vm.prank(owner); - configHub.setPauseGuardian(user1, true); - vm.prank(user1); - testedContract.pauseByGuardian(); - assertTrue(testedContract.paused()); + // has no owner view, so reverts when trying to get owner() + vm.expectRevert(new bytes(0)); + testedContract.setConfigHub(ConfigHub(address(testedContract))); } - function test_revert_pauseByGuardian_ownerRenounced() public { - vm.prank(owner); - configHub.setPauseGuardian(guardian, true); - - vm.prank(owner); - testedContract.renounceOwnership(); + function test_onlyConfigHubOwnerMethods() public { + startHoax(user1); - vm.prank(guardian); - vm.expectRevert("owner renounced"); - testedContract.pauseByGuardian(); - } + vm.expectRevert("BaseManaged: not configHub owner"); + testedContract.setConfigHub(configHub); - function test_revert_guardian_unpause() public { - vm.prank(owner); - configHub.setPauseGuardian(guardian, true); - - vm.startPrank(guardian); - // can pause - testedContract.pauseByGuardian(); - // cannot unpause - vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, guardian)); - testedContract.unpause(); + vm.expectRevert("BaseManaged: not configHub owner"); + testedContract.rescueTokens(address(0), 0, true); } function test_unrescuable() public { @@ -229,33 +151,37 @@ contract BadConfigHub1 { } contract BadConfigHub2 { + address public owner; + + constructor(address _owner) { + owner = _owner; + } + function VERSION() external returns (string memory) { } } // mock of an inheriting contract (because base is abstract) contract TestableBaseManaged is BaseManaged { - constructor(address _initialOwner, ConfigHub _configHub, address _unrescuable) - BaseManaged(_initialOwner, _configHub, _unrescuable) - { } + constructor(ConfigHub _configHub, address _unrescuable) BaseManaged(_configHub, _unrescuable) { } } // the tests for the mock contract contract BaseManagedMockTest is BaseManagedTestBase { function setupTestedContract() internal override { - testedContract = new TestableBaseManaged(owner, configHub, unrescuable); + testedContract = new TestableBaseManaged(configHub, unrescuable); } function test_revert_constructor_invalidConfigHub() public { vm.expectRevert(new bytes(0)); - new TestableBaseManaged(owner, ConfigHub(address(0)), unrescuable); + new TestableBaseManaged(ConfigHub(address(0)), unrescuable); ConfigHub badHub = ConfigHub(address(new BadConfigHub1())); vm.expectRevert(); - new TestableBaseManaged(owner, badHub, unrescuable); + new TestableBaseManaged(badHub, unrescuable); - badHub = ConfigHub(address(new BadConfigHub2())); + badHub = ConfigHub(address(new BadConfigHub2(owner))); vm.expectRevert("invalid ConfigHub"); - new TestableBaseManaged(owner, badHub, unrescuable); + new TestableBaseManaged(badHub, unrescuable); } } @@ -264,7 +190,7 @@ contract ProviderNFTManagedTest is BaseManagedTestBase { TestERC20 unrescuableErc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); unrescuable = address(unrescuableErc20); testedContract = new CollarProviderNFT( - owner, configHub, unrescuableErc20, erc20Rescuable, address(0), "ProviderNFT", "ProviderNFT" + configHub, unrescuableErc20, erc20Rescuable, address(0), "ProviderNFT", "ProviderNFT" ); } } @@ -273,8 +199,7 @@ contract EscrowSupplierNFTManagedTest is BaseManagedTestBase { function setupTestedContract() internal override { TestERC20 unrescuableErc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); unrescuable = address(unrescuableErc20); - testedContract = - new EscrowSupplierNFT(owner, configHub, unrescuableErc20, "ProviderNFT", "ProviderNFT"); + testedContract = new EscrowSupplierNFT(configHub, unrescuableErc20, "ProviderNFT", "ProviderNFT"); } } @@ -294,7 +219,7 @@ contract TakerNFTManagedTest is BaseManagedTestBase { // taker checks price on construction mockCLFeed.setLatestAnswer(1, 0); testedContract = new CollarTakerNFT( - owner, configHub, unrescuableErc20, erc20Rescuable, oracle, "CollarTakerNFT", "BRWTST" + configHub, unrescuableErc20, erc20Rescuable, oracle, "CollarTakerNFT", "BRWTST" ); } } @@ -305,16 +230,6 @@ contract LoansManagedTest is TakerNFTManagedTest { // take the taker contract setup by the super CollarTakerNFT takerNFT = CollarTakerNFT(address(testedContract)); unrescuable = address(takerNFT); - testedContract = new LoansNFT(owner, takerNFT, "", ""); - } -} - -contract RollsManagedTest is TakerNFTManagedTest { - function setupTestedContract() internal override { - super.setupTestedContract(); - // take the taker contract setup by the super - CollarTakerNFT takerNFT = CollarTakerNFT(address(testedContract)); - unrescuable = address(0); - testedContract = new Rolls(owner, takerNFT); + testedContract = new LoansNFT(takerNFT, "", ""); } } diff --git a/test/unit/CollarProviderNFT.t.sol b/test/unit/CollarProviderNFT.t.sol index 339bcf16..5f9e6286 100644 --- a/test/unit/CollarProviderNFT.t.sol +++ b/test/unit/CollarProviderNFT.t.sol @@ -4,7 +4,6 @@ pragma solidity 0.8.22; import "forge-std/Test.sol"; import { IERC721Errors, Strings } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; -import { Pausable } from "@openzeppelin/contracts/utils/Pausable.sol"; import { TestERC20 } from "../utils/TestERC20.sol"; import { BaseAssetPairTestSetup } from "./BaseAssetPairTestSetup.sol"; @@ -130,10 +129,10 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { function test_constructor() public { CollarProviderNFT newProviderNFT = new CollarProviderNFT( - owner, configHub, cashAsset, underlying, address(takerContract), "NewCollarProviderNFT", "NPRVNFT" + configHub, cashAsset, underlying, address(takerContract), "NewCollarProviderNFT", "NPRVNFT" ); - assertEq(address(newProviderNFT.owner()), owner); + assertEq(address(newProviderNFT.configHubOwner()), owner); assertEq(address(newProviderNFT.configHub()), address(configHub)); assertEq(newProviderNFT.unrescuableAsset(), address(cashAsset)); assertEq(address(newProviderNFT.cashAsset()), address(cashAsset)); @@ -143,7 +142,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { assertEq(newProviderNFT.MAX_CALL_STRIKE_BIPS(), 100_000); assertEq(newProviderNFT.MAX_PUT_STRIKE_BIPS(), 9999); assertEq(newProviderNFT.MAX_PROTOCOL_FEE_BIPS(), 100); - assertEq(newProviderNFT.VERSION(), "0.2.0"); + assertEq(newProviderNFT.VERSION(), "0.3.0"); assertEq(newProviderNFT.name(), "NewCollarProviderNFT"); assertEq(newProviderNFT.symbol(), "NPRVNFT"); } @@ -498,17 +497,6 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { providerNFT.cancelAndWithdraw(positionId); } - function test_unpause() public { - vm.startPrank(owner); - providerNFT.pause(); - vm.expectEmit(address(providerNFT)); - emit Pausable.Unpaused(owner); - providerNFT.unpause(); - assertFalse(providerNFT.paused()); - // check at least one method workds now - createAndCheckOffer(provider, largeCash); - } - /// Interactions between multiple items function test_createMultipleOffersFromSameProvider() public { @@ -612,39 +600,6 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { /// Reverts - function test_pausableMethods() public { - // create a position - (uint positionId,) = createAndCheckPosition(provider, largeCash, largeCash / 2); - - // pause - vm.startPrank(owner); - vm.expectEmit(address(providerNFT)); - emit Pausable.Paused(owner); - providerNFT.pause(); - // paused view - assertTrue(providerNFT.paused()); - // methods are paused - vm.startPrank(provider); - - vm.expectRevert(Pausable.EnforcedPause.selector); - providerNFT.createOffer(0, 0, 0, 0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - providerNFT.updateOfferAmount(0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - providerNFT.mintFromOffer(0, 0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - providerNFT.settlePosition(0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - providerNFT.withdrawFromSettled(0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - providerNFT.cancelAndWithdraw(0); - } - function test_revert_createOffer_invalidParams() public { uint minStrike = providerNFT.MIN_CALL_STRIKE_BIPS(); vm.expectRevert("provider: strike percent too low"); diff --git a/test/unit/CollarTakerNFT.t.sol b/test/unit/CollarTakerNFT.t.sol index 4bd930a2..b8cc4056 100644 --- a/test/unit/CollarTakerNFT.t.sol +++ b/test/unit/CollarTakerNFT.t.sol @@ -223,17 +223,16 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { vm.expectEmit(); emit ICollarTakerNFT.OracleSet(BaseTakerOracle(address(0)), chainlinkOracle); CollarTakerNFT takerNFT = new CollarTakerNFT( - owner, configHub, cashAsset, underlying, chainlinkOracle, "NewCollarTakerNFT", "NBPNFT" + configHub, cashAsset, underlying, chainlinkOracle, "NewCollarTakerNFT", "NBPNFT" ); - assertEq(takerNFT.owner(), owner); - assertEq(takerNFT.pendingOwner(), address(0)); + assertEq(takerNFT.configHubOwner(), owner); assertEq(address(takerNFT.configHub()), address(configHub)); assertEq(takerNFT.unrescuableAsset(), address(cashAsset)); assertEq(address(takerNFT.cashAsset()), address(cashAsset)); assertEq(address(takerNFT.underlying()), address(underlying)); assertEq(address(takerNFT.oracle()), address(chainlinkOracle)); - assertEq(takerNFT.VERSION(), "0.2.0"); + assertEq(takerNFT.VERSION(), "0.3.0"); assertEq(takerNFT.name(), "NewCollarTakerNFT"); assertEq(takerNFT.symbol(), "NBPNFT"); assertEq(takerNFT.nextPositionId(), 1); @@ -243,79 +242,38 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { // Create an oracle with mismatched assets BaseTakerOracle invalidOracle = createMockFeedOracle(address(cashAsset), address(underlying)); vm.expectRevert("taker: oracle underlying mismatch"); - new CollarTakerNFT( - owner, configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT" - ); + new CollarTakerNFT(configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT"); invalidOracle = createMockFeedOracle(address(cashAsset), address(cashAsset)); vm.expectRevert("taker: oracle underlying mismatch"); - new CollarTakerNFT( - owner, configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT" - ); + new CollarTakerNFT(configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT"); invalidOracle = createMockFeedOracle(address(underlying), address(underlying)); vm.expectRevert("taker: oracle cashAsset mismatch"); - new CollarTakerNFT( - owner, configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT" - ); + new CollarTakerNFT(configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT"); vm.mockCall(address(100), abi.encodeCall(IERC20Metadata.decimals, ()), abi.encode(18)); invalidOracle = createMockFeedOracle(address(underlying), address(100)); vm.expectRevert("taker: oracle cashAsset mismatch"); - new CollarTakerNFT( - owner, configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT" - ); + new CollarTakerNFT(configHub, cashAsset, underlying, invalidOracle, "NewCollarTakerNFT", "NBPNFT"); BaseTakerOracle newOracle = createMockFeedOracle(address(underlying), address(cashAsset)); vm.mockCall(address(newOracle), abi.encodeCall(ITakerOracle.currentPrice, ()), abi.encode(0)); vm.expectRevert("taker: invalid current price"); - new CollarTakerNFT(owner, configHub, cashAsset, underlying, newOracle, "NewCollarTakerNFT", "NBPNFT"); + new CollarTakerNFT(configHub, cashAsset, underlying, newOracle, "NewCollarTakerNFT", "NBPNFT"); vm.clearMockedCalls(); updatePrice(1); // 0 conversion view vm.mockCall(address(newOracle), abi.encodeCall(newOracle.convertToBaseAmount, (1, 1)), abi.encode(0)); vm.expectRevert("taker: invalid convertToBaseAmount"); - new CollarTakerNFT(owner, configHub, cashAsset, underlying, newOracle, "NewCollarTakerNFT", "NBPNFT"); + new CollarTakerNFT(configHub, cashAsset, underlying, newOracle, "NewCollarTakerNFT", "NBPNFT"); vm.clearMockedCalls(); // reverting conversion view vm.mockCallRevert(address(newOracle), abi.encodeCall(newOracle.convertToBaseAmount, (1, 1)), "mocked"); vm.expectRevert("mocked"); - new CollarTakerNFT(owner, configHub, cashAsset, underlying, newOracle, "NewCollarTakerNFT", "NBPNFT"); - } - - function test_pausableMethods() public { - // create a position - checkOpenPairedPosition(); - - startHoax(owner); - takerNFT.pause(); - assertTrue(takerNFT.paused()); - - // Try to open a position while paused - vm.expectRevert(Pausable.EnforcedPause.selector); - takerNFT.openPairedPosition(takerLocked, providerNFT, 0); - // Try to settle a position while paused - vm.expectRevert(Pausable.EnforcedPause.selector); - takerNFT.settlePairedPosition(0); - - // Try to withdraw from settled while paused - vm.expectRevert(Pausable.EnforcedPause.selector); - takerNFT.withdrawFromSettled(0); - - // Try to cancel a paired position while paused - vm.expectRevert(Pausable.EnforcedPause.selector); - takerNFT.cancelPairedPosition(0); - } - - function test_unpause() public { - startHoax(owner); - takerNFT.pause(); - takerNFT.unpause(); - assertFalse(takerNFT.paused()); - // Should be able to open a position after unpausing - checkOpenPairedPosition(); + new CollarTakerNFT(configHub, cashAsset, underlying, newOracle, "NewCollarTakerNFT", "NBPNFT"); } function test_supportsInterface() public view { @@ -466,7 +424,7 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { // cash mismatch CollarProviderNFT providerNFTBad = new CollarProviderNFT( - owner, configHub, underlying, underlying, address(takerNFT), "CollarTakerNFTBad", "BRWTSTBAD" + configHub, underlying, underlying, address(takerNFT), "CollarTakerNFTBad", "BRWTSTBAD" ); setCanOpen(address(providerNFTBad), true); startHoax(user1); @@ -476,7 +434,7 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { // underlying mismatch providerNFTBad = new CollarProviderNFT( - owner, configHub, cashAsset, cashAsset, address(takerNFT), "CollarTakerNFTBad", "BRWTSTBAD" + configHub, cashAsset, cashAsset, address(takerNFT), "CollarTakerNFTBad", "BRWTSTBAD" ); setCanOpen(address(providerNFTBad), true); startHoax(user1); @@ -486,7 +444,7 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { // taker mismatch providerNFTBad = new CollarProviderNFT( - owner, configHub, cashAsset, underlying, address(providerNFT2), "CollarTakerNFTBad", "BRWTSTBAD" + configHub, cashAsset, underlying, address(providerNFT2), "CollarTakerNFTBad", "BRWTSTBAD" ); setCanOpen(address(providerNFTBad), true); startHoax(user1); @@ -788,7 +746,7 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { function test_revert_setOracle() public { startHoax(user1); - vm.expectRevert(abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, user1)); + vm.expectRevert("BaseManaged: not configHub owner"); takerNFT.setOracle(chainlinkOracle); startHoax(owner); diff --git a/test/unit/EscrowSupplier.admin.t.sol b/test/unit/EscrowSupplier.admin.t.sol index 87c5f6c6..740e631b 100644 --- a/test/unit/EscrowSupplier.admin.t.sol +++ b/test/unit/EscrowSupplier.admin.t.sol @@ -9,17 +9,10 @@ import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; import { BaseEscrowSupplierNFTTest, IEscrowSupplierNFT } from "./EscrowSupplierNFT.effects.t.sol"; contract EscrowSupplierNFT_AdminTest is BaseEscrowSupplierNFTTest { - function test_onlyOwnerMethods() public { + function test_onlyConfigHubOwnerMethods() public { vm.startPrank(user1); - bytes4 selector = Ownable.OwnableUnauthorizedAccount.selector; - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - escrowNFT.pause(); - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - escrowNFT.unpause(); - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); + vm.expectRevert("BaseManaged: not configHub owner"); escrowNFT.setLoansCanOpen(loans, true); } @@ -41,50 +34,4 @@ contract EscrowSupplierNFT_AdminTest is BaseEscrowSupplierNFTTest { assertFalse(escrowNFT.loansCanOpen(newLoansContract)); } - - function test_paused_methods() public { - (uint escrowId,) = createAndCheckEscrow(supplier1, largeUnderlying, largeUnderlying, escrowFee); - - // pause - vm.startPrank(owner); - vm.expectEmit(address(escrowNFT)); - emit Pausable.Paused(owner); - escrowNFT.pause(); - // paused view - assertTrue(escrowNFT.paused()); - // methods are paused - vm.startPrank(user1); - - vm.expectRevert(Pausable.EnforcedPause.selector); - escrowNFT.createOffer(0, 0, 0, 0, 0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - escrowNFT.updateOfferAmount(0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - escrowNFT.startEscrow(0, 0, 0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - escrowNFT.endEscrow(0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - escrowNFT.switchEscrow(0, 0, 0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - escrowNFT.withdrawReleased(0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - escrowNFT.seizeEscrow(0); - } - - function test_unpause() public { - vm.startPrank(owner); - escrowNFT.pause(); - vm.expectEmit(address(escrowNFT)); - emit Pausable.Unpaused(owner); - escrowNFT.unpause(); - assertFalse(escrowNFT.paused()); - // check at least one method workds now - createAndCheckOffer(supplier1, largeUnderlying); - } } diff --git a/test/unit/EscrowSupplierNFT.effects.t.sol b/test/unit/EscrowSupplierNFT.effects.t.sol index 7944c1dc..0bb810a6 100644 --- a/test/unit/EscrowSupplierNFT.effects.t.sol +++ b/test/unit/EscrowSupplierNFT.effects.t.sol @@ -28,7 +28,7 @@ contract BaseEscrowSupplierNFTTest is BaseAssetPairTestSetup { function setUp() public override { super.setUp(); asset = underlying; - escrowNFT = new EscrowSupplierNFT(owner, configHub, asset, "ES Test", "ES Test"); + escrowNFT = new EscrowSupplierNFT(configHub, asset, "ES Test", "ES Test"); setCanOpenSingle(address(escrowNFT), true); setCanOpen(loans, true); @@ -284,9 +284,9 @@ contract BaseEscrowSupplierNFTTest is BaseAssetPairTestSetup { contract EscrowSupplierNFT_BasicEffectsTest is BaseEscrowSupplierNFTTest { function test_constructor() public { EscrowSupplierNFT newEscrowSupplierNFT = - new EscrowSupplierNFT(owner, configHub, asset, "NewEscrowSupplierNFT", "NESNFT"); + new EscrowSupplierNFT(configHub, asset, "NewEscrowSupplierNFT", "NESNFT"); - assertEq(address(newEscrowSupplierNFT.owner()), owner); + assertEq(address(newEscrowSupplierNFT.configHubOwner()), owner); assertEq(address(newEscrowSupplierNFT.configHub()), address(configHub)); assertEq(newEscrowSupplierNFT.unrescuableAsset(), address(asset)); assertEq(address(newEscrowSupplierNFT.asset()), address(asset)); @@ -295,7 +295,7 @@ contract EscrowSupplierNFT_BasicEffectsTest is BaseEscrowSupplierNFTTest { assertEq(newEscrowSupplierNFT.MAX_GRACE_PERIOD(), 30 days); assertEq(newEscrowSupplierNFT.MAX_LATE_FEE_APR_BIPS(), 12 * BIPS_100PCT); assertEq(newEscrowSupplierNFT.MAX_FEE_REFUND_BIPS(), 9500); - assertEq(newEscrowSupplierNFT.VERSION(), "0.2.0"); + assertEq(newEscrowSupplierNFT.VERSION(), "0.3.0"); assertEq(newEscrowSupplierNFT.name(), "NewEscrowSupplierNFT"); assertEq(newEscrowSupplierNFT.symbol(), "NESNFT"); } diff --git a/test/unit/Loans.admin.t.sol b/test/unit/Loans.admin.t.sol index 5fb75d2d..61ea06f4 100644 --- a/test/unit/Loans.admin.t.sol +++ b/test/unit/Loans.admin.t.sol @@ -17,20 +17,13 @@ import { } from "./Loans.basic.effects.t.sol"; contract LoansAdminTest is LoansTestBase { - function test_onlyOwnerMethods() public { + function test_onlyConfigHubOwnerMethods() public { vm.startPrank(user1); - bytes4 selector = Ownable.OwnableUnauthorizedAccount.selector; - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - loans.pause(); - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); - loans.unpause(); - - vm.expectRevert(abi.encodeWithSelector(selector, user1)); + vm.expectRevert("BaseManaged: not configHub owner"); loans.setKeeper(keeper); - vm.expectRevert(abi.encodeWithSelector(selector, user1)); + vm.expectRevert("BaseManaged: not configHub owner"); loans.setSwapperAllowed(address(0), true, true); } @@ -89,49 +82,6 @@ contract LoansAdminTest is LoansTestBase { assertEq(loans.defaultSwapper(), address(0)); // default is unset } - function test_pause() public { - (uint loanId,,) = createAndCheckLoan(); - - // pause - vm.startPrank(owner); - vm.expectEmit(address(loans)); - emit Pausable.Paused(owner); - loans.pause(); - // paused view - assertTrue(loans.paused()); - // methods are paused - vm.startPrank(user1); - - vm.expectRevert(Pausable.EnforcedPause.selector); - loans.openLoan(0, 0, defaultSwapParams(0), providerOffer(0)); - - vm.expectRevert(Pausable.EnforcedPause.selector); - loans.openEscrowLoan(0, 0, defaultSwapParams(0), providerOffer(0), escrowOffer(0), 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - loans.setKeeperApproved(0, true); - - vm.expectRevert(Pausable.EnforcedPause.selector); - loans.closeLoan(loanId, defaultSwapParams(0)); - - vm.expectRevert(Pausable.EnforcedPause.selector); - loans.rollLoan(loanId, rollOffer(0), 0, 0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - loans.unwrapAndCancelLoan(loanId); - } - - function test_unpause() public { - vm.startPrank(owner); - loans.pause(); - vm.expectEmit(address(loans)); - emit Pausable.Unpaused(owner); - loans.unpause(); - assertFalse(loans.paused()); - // check at least one method workds now - createAndCheckLoan(); - } - function test_setSwapperAllowed_InvalidSwapper() public { address invalidSwapper = address(0x456); diff --git a/test/unit/Loans.basic.effects.t.sol b/test/unit/Loans.basic.effects.t.sol index 89507fa0..f43560d1 100644 --- a/test/unit/Loans.basic.effects.t.sol +++ b/test/unit/Loans.basic.effects.t.sol @@ -56,10 +56,10 @@ contract LoansTestBase is BaseAssetPairTestSetup { vm.label(address(mockSwapperRouter), "MockSwapRouter"); vm.label(address(swapperUniV3), "SwapperUniV3"); // escrow - escrowNFT = new EscrowSupplierNFT(owner, configHub, underlying, "Escrow", "Escrow"); + escrowNFT = new EscrowSupplierNFT(configHub, underlying, "Escrow", "Escrow"); vm.label(address(escrowNFT), "Escrow"); // loans - loans = new LoansNFT(owner, takerNFT, "Loans", "Loans"); + loans = new LoansNFT(takerNFT, "Loans", "Loans"); vm.label(address(loans), "Loans"); // config @@ -182,7 +182,8 @@ contract LoansTestBase is BaseAssetPairTestSetup { uint expectedLoanAmount = swapOut * ltv / BIPS_100PCT; uint expectedProviderLocked = swapOut * (callStrikePercent - BIPS_100PCT) / BIPS_100PCT; - (uint expectedProtocolFee,) = providerNFT.protocolFee(expectedProviderLocked, duration, callStrikePercent); + (uint expectedProtocolFee,) = + providerNFT.protocolFee(expectedProviderLocked, duration, callStrikePercent); if (expectedProviderLocked != 0) assertGt(expectedProtocolFee, 0); // ensure fee is expected ILoansNFT.SwapParams memory swapParams = defaultSwapParams(swapCashAmount); @@ -437,15 +438,15 @@ contract LoansBasicEffectsTest is LoansTestBase { // tests function test_constructor() public { - loans = new LoansNFT(owner, takerNFT, "", ""); + loans = new LoansNFT(takerNFT, "", ""); assertEq(loans.MAX_SWAP_PRICE_DEVIATION_BIPS(), 1000); assertEq(address(loans.configHub()), address(configHub)); assertEq(loans.unrescuableAsset(), address(takerNFT)); assertEq(address(loans.takerNFT()), address(takerNFT)); assertEq(address(loans.cashAsset()), address(cashAsset)); assertEq(address(loans.underlying()), address(underlying)); - assertEq(loans.VERSION(), "0.2.0"); - assertEq(loans.owner(), owner); + assertEq(loans.VERSION(), "0.3.0"); + assertEq(loans.configHubOwner(), owner); assertEq(loans.closingKeeper(), address(0)); assertEq(address(loans.defaultSwapper()), address(0)); assertEq(loans.name(), ""); diff --git a/test/unit/Loans.escrow.reverts.t.sol b/test/unit/Loans.escrow.reverts.t.sol index 4cf8ec7c..57b52c68 100644 --- a/test/unit/Loans.escrow.reverts.t.sol +++ b/test/unit/Loans.escrow.reverts.t.sol @@ -45,7 +45,7 @@ contract LoansEscrowRevertsTest is LoansBasicRevertsTest { openLoan(underlyingAmount, minLoanAmount, 0, 0); // test escrow asset mismatch - EscrowSupplierNFT invalidEscrow = new EscrowSupplierNFT(owner, configHub, cashAsset, "", ""); + EscrowSupplierNFT invalidEscrow = new EscrowSupplierNFT(configHub, cashAsset, "", ""); setCanOpenSingle(address(invalidEscrow), true); vm.startPrank(user1); escrowNFT = invalidEscrow; diff --git a/test/unit/Rolls.t.sol b/test/unit/Rolls.t.sol index 42f7daed..96230328 100644 --- a/test/unit/Rolls.t.sol +++ b/test/unit/Rolls.t.sol @@ -131,7 +131,8 @@ contract RollsTest is BaseAssetPairTestSetup { takerNFT.calculateProviderLocked(expected.newTakerLocked, ltv, callStrikePercent) ); // protocol fee - (expected.toProtocol,) = providerNFT.protocolFee(expected.newProviderLocked, duration, callStrikePercent); + (expected.toProtocol,) = + providerNFT.protocolFee(expected.newProviderLocked, duration, callStrikePercent); // _calculateTransferAmounts (uint takerSettled, int providerChange) = takerNFT.previewSettlement(oldTakerPos, newPrice); int providerSettled = int(oldTakerPos.providerLocked) + providerChange; @@ -274,13 +275,10 @@ contract RollsTest is BaseAssetPairTestSetup { // happy cases function test_constructor() public { - Rolls newRolls = new Rolls(owner, takerNFT); + Rolls newRolls = new Rolls(takerNFT); assertEq(address(newRolls.takerNFT()), address(takerNFT)); - assertEq(address(newRolls.configHub()), address(configHub)); - assertEq(newRolls.unrescuableAsset(), address(0)); assertEq(address(newRolls.cashAsset()), address(cashAsset)); - assertEq(newRolls.VERSION(), "0.2.0"); - assertEq(newRolls.owner(), owner); + assertEq(newRolls.VERSION(), "0.3.0"); } function test_createRollOffer() public { @@ -516,38 +514,6 @@ contract RollsTest is BaseAssetPairTestSetup { ); } - function test_pause() public { - // pause - vm.startPrank(owner); - vm.expectEmit(address(rolls)); - emit Pausable.Paused(owner); - rolls.pause(); - // paused view - assertTrue(rolls.paused()); - // methods are paused - vm.startPrank(user1); - - vm.expectRevert(Pausable.EnforcedPause.selector); - rolls.createOffer(0, 0, 0, 0, 0, 0, 0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - rolls.cancelOffer(0); - - vm.expectRevert(Pausable.EnforcedPause.selector); - rolls.executeRoll(0, 0); - } - - function test_unpause() public { - vm.startPrank(owner); - rolls.pause(); - vm.expectEmit(address(rolls)); - emit Pausable.Unpaused(owner); - rolls.unpause(); - assertFalse(rolls.paused()); - // check at least one method works now - createAndCheckRollOffer(); - } - // reverts function test_revert_createRollOffer() public { From f7cb8b4e260b87882610a15e17769eba57091cfd Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Wed, 2 Apr 2025 12:26:09 +1100 Subject: [PATCH 03/12] Ownership reduction part 2 - remove setOracle, add settleAsCancelled (#139) * update view and local tests * add changelog * minor * remove pausing and ownership in basemanaged * remove pausing and ownership for rolls * remove rest of pausing and ownership, fix tests * update changelog * update version string * add version string check in fork tests * remove setOracle, add settleAsCancelled * add note re loans * remove event * make oracle immutable --- CHANGELOG.md | 2 + src/CollarTakerNFT.sol | 76 +++++++++---- src/interfaces/ICollarTakerNFT.sol | 1 - test/unit/CollarTakerNFT.t.sol | 170 +++++++++++++++++------------ 4 files changed, 159 insertions(+), 90 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 49278f18..c027fc81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ Notable changes to contracts. - Inheriting from updated BaseManaged (not ownable, not pausable). - Different constructor interface. - Using `onlyConfigHubOwner` for admin methods. +- CollarTakerNFT: + - `setOracle` is removed, and `settleAsCancelled` (for oracle failure) is added - CollarProviderNFT: - `protocolFee` charges fee on full notional amount (cash value of loan input), and expects `callStrikePercent` argument. - Rolls: diff --git a/src/CollarTakerNFT.sol b/src/CollarTakerNFT.sol index b119d84c..03618255 100644 --- a/src/CollarTakerNFT.sol +++ b/src/CollarTakerNFT.sol @@ -46,15 +46,18 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { uint internal constant BIPS_BASE = 10_000; + /// @notice delay after expiry when positions can be settled to their original locked values. + /// This is needed for the case of a prolonged oracle failure. + uint public constant SETTLE_AS_CANCELLED_DELAY = 1 weeks; + string public constant VERSION = "0.3.0"; // ----- IMMUTABLES ----- // IERC20 public immutable cashAsset; IERC20 public immutable underlying; // not used as ERC20 here + ITakerOracle public immutable oracle; // ----- STATE VARIABLES ----- // - ITakerOracle public oracle; - mapping(uint positionId => TakerPositionStored) internal positions; constructor( @@ -67,7 +70,8 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { ) BaseNFT(_name, _symbol, _configHub, address(_cashAsset)) { cashAsset = _cashAsset; underlying = _underlying; - _setOracle(_oracle); + _checkOracle(_oracle); + oracle = _oracle; } // ----- VIEW FUNCTIONS ----- // @@ -265,6 +269,53 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { ); } + /** + * @notice Settles a paired position after expiry + SETTLE_AS_CANCELLED_DELAY if + * settlePairedPosition was not called during the delay by anyone, presumably due to oracle failure. + * @param takerId The ID of the taker position to settle + * + * @dev this method exists because if the oracle starts reverting, there is no way to update it. + * Either taker or provider are incentivised to call settlePairedPosition prior to this method being callable, + * and anyone else (like a keeper), can call it too. So if it wasn't called by anyone, we need to settle the + * positions without an oracle, and allow their withdrawal. + * Only callable by a holder of either the taker or provider positions. If the NFT is held + * by another contract it needs to allow withdrawing the NFT to call this method. For example + * LoansNFT.unwrapAndCancelLoan may need to be used in Loans by the borrower to call this method. + */ + function settleAsCancelled(uint takerId) external nonReentrant { + // @dev this checks position exists + TakerPosition memory position = getPosition(takerId); + (CollarProviderNFT providerNFT, uint providerId) = (position.providerNFT, position.providerId); + + require( + block.timestamp >= position.expiration + SETTLE_AS_CANCELLED_DELAY, + "taker: cannot be settled as cancelled yet" + ); + require(!position.settled, "taker: already settled"); + + // must be taker or provider NFT owner to call this. ownerOf can revert if NFT is burned, + // but that can only happen after successful settlement. + require( + msg.sender == ownerOf(takerId) || msg.sender == providerNFT.ownerOf(providerId), + "taker: not owner of taker or provider position" + ); + + // store changes + positions[takerId].settled = true; + positions[takerId].withdrawable = position.takerLocked; + + uint expectedBalance = cashAsset.balanceOf(address(this)); + // call provider side to settle with no balance change + providerNFT.settlePosition(providerId, 0); + // check balance update to prevent reducing resting balance + require(cashAsset.balanceOf(address(this)) == expectedBalance, "taker: settle balance mismatch"); + + // endPrice is 0 since is unavailable, technically settlement price is position.startPrice but + // it would be incorrect to pass it as endPrice. + // Also, 0 allows distinguishing this type of settlement in events. + emit PairedPositionSettled(takerId, address(providerNFT), providerId, 0, position.takerLocked, 0); + } + /// @notice Withdraws funds from a settled position. Burns the NFT. /// @param takerId The ID of the settled position to withdraw from (NFT token ID). /// @return withdrawal The amount of cash asset withdrawn @@ -326,19 +377,9 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { ); } - // ----- admin Mutative ----- // - - /// @notice Sets the price oracle used by the contract - /// @param _oracle The new price oracle to use - function setOracle(ITakerOracle _oracle) external onlyConfigHubOwner { - _setOracle(_oracle); - } - - // ----- INTERNAL MUTATIVE ----- // - - // internal owner + // ----- INTERNAL VIEWS ----- // - function _setOracle(ITakerOracle _oracle) internal { + function _checkOracle(ITakerOracle _oracle) internal view { // assets match require(_oracle.baseToken() == address(underlying), "taker: oracle underlying mismatch"); require(_oracle.quoteToken() == address(cashAsset), "taker: oracle cashAsset mismatch"); @@ -354,13 +395,8 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { // note: .convertToBaseAmount(price, price) should equal .baseUnitAmount(), but checking this // may be too strict for more complex oracles, and .baseUnitAmount() is not used internally now require(_oracle.convertToBaseAmount(price, price) != 0, "taker: invalid convertToBaseAmount"); - - emit OracleSet(oracle, _oracle); // emit before for the prev value - oracle = _oracle; } - // ----- INTERNAL VIEWS ----- // - // calculations // Rounding down precision loss is negligible, and the values (strike percentages, and price) diff --git a/src/interfaces/ICollarTakerNFT.sol b/src/interfaces/ICollarTakerNFT.sol index 039f3674..4c35a0d6 100644 --- a/src/interfaces/ICollarTakerNFT.sol +++ b/src/interfaces/ICollarTakerNFT.sol @@ -60,5 +60,4 @@ interface ICollarTakerNFT { uint expiration ); event WithdrawalFromSettled(uint indexed takerId, uint withdrawn); - event OracleSet(ITakerOracle prevOracle, ITakerOracle newOracle); } diff --git a/test/unit/CollarTakerNFT.t.sol b/test/unit/CollarTakerNFT.t.sol index b8cc4056..8973addf 100644 --- a/test/unit/CollarTakerNFT.t.sol +++ b/test/unit/CollarTakerNFT.t.sol @@ -206,22 +206,60 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { assertEq(providerPosAfter.withdrawable, expectedProviderOut); } + function checkSettleAsCancelled(uint takerId, address caller) internal { + CollarTakerNFT.TakerPosition memory takerPos = takerNFT.getPosition(takerId); + uint providerNFTId = takerPos.providerId; + uint takerNFTBalanceBefore = cashAsset.balanceOf(address(takerNFT)); + uint providerNFTBalanceBefore = cashAsset.balanceOf(address(providerNFT)); + + startHoax(caller); + vm.expectEmit(address(providerNFT)); + emit ICollarProviderNFT.PositionSettled(providerNFTId, 0, providerLocked); + vm.expectEmit(address(takerNFT)); + emit ICollarTakerNFT.PairedPositionSettled( + takerId, address(providerNFT), providerNFTId, 0, takerLocked, 0 + ); + takerNFT.settleAsCancelled(takerId); + + // balance changes + assertEq(cashAsset.balanceOf(address(takerNFT)), takerNFTBalanceBefore); + assertEq(cashAsset.balanceOf(address(providerNFT)), providerNFTBalanceBefore); + + // positions changes + CollarTakerNFT.TakerPosition memory takerPosAfter = takerNFT.getPosition(takerId); + assertEq(takerPosAfter.settled, true); + assertEq(takerPosAfter.withdrawable, takerLocked); + (, bool settled) = takerNFT.expirationAndSettled(takerId); + assertEq(settled, true); + + CollarProviderNFT.ProviderPosition memory providerPosAfter = providerNFT.getPosition(providerNFTId); + assertEq(providerPosAfter.settled, true); + assertEq(providerPosAfter.withdrawable, providerLocked); + } + function checkWithdrawFromSettled(uint takerId, uint expectedTakerOut) public { - uint cashBalanceBefore = cashAsset.balanceOf(user1); + startHoax(user1); + uint takerCashBefore = cashAsset.balanceOf(user1); vm.expectEmit(address(takerNFT)); emit ICollarTakerNFT.WithdrawalFromSettled(takerId, expectedTakerOut); - uint withdrawal = takerNFT.withdrawFromSettled(takerId); - assertEq(withdrawal, expectedTakerOut); - assertEq(cashAsset.balanceOf(user1), cashBalanceBefore + expectedTakerOut); - CollarTakerNFT.TakerPosition memory position = takerNFT.getPosition(takerId); - assertEq(position.withdrawable, 0); + uint withdrawalTaker = takerNFT.withdrawFromSettled(takerId); + assertEq(withdrawalTaker, expectedTakerOut); + assertEq(cashAsset.balanceOf(user1), takerCashBefore + expectedTakerOut); + assertEq(takerNFT.getPosition(takerId).withdrawable, 0); + + startHoax(provider); + uint providerCashBefore = cashAsset.balanceOf(provider); + uint providerId = takerNFT.getPosition(takerId).providerId; + uint withdrawalProvider = providerNFT.withdrawFromSettled(providerId); + uint expectedProviderOut = providerLocked + takerLocked - expectedTakerOut; + assertEq(withdrawalProvider, expectedProviderOut); + assertEq(cashAsset.balanceOf(provider), providerCashBefore + expectedProviderOut); + assertEq(providerNFT.getPosition(providerId).withdrawable, 0); } // tests function test_constructor() public { - vm.expectEmit(); - emit ICollarTakerNFT.OracleSet(BaseTakerOracle(address(0)), chainlinkOracle); CollarTakerNFT takerNFT = new CollarTakerNFT( configHub, cashAsset, underlying, chainlinkOracle, "NewCollarTakerNFT", "NBPNFT" ); @@ -232,6 +270,7 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { assertEq(address(takerNFT.cashAsset()), address(cashAsset)); assertEq(address(takerNFT.underlying()), address(underlying)); assertEq(address(takerNFT.oracle()), address(chainlinkOracle)); + assertEq(takerNFT.SETTLE_AS_CANCELLED_DELAY(), 1 weeks); assertEq(takerNFT.VERSION(), "0.3.0"); assertEq(takerNFT.name(), "NewCollarTakerNFT"); assertEq(takerNFT.symbol(), "NBPNFT"); @@ -603,6 +642,60 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { takerNFT.settlePairedPosition(takerId); } + function test_settleAsCancelled() public { + (uint takerId,) = checkOpenPairedPosition(); + skip(duration + takerNFT.SETTLE_AS_CANCELLED_DELAY()); + // oracle reverts now + vm.mockCallRevert(address(takerNFT.oracle()), abi.encodeCall(takerNFT.oracle().currentPrice, ()), ""); + // check that it reverts + vm.expectRevert(new bytes(0)); + takerNFT.currentOraclePrice(); + + uint snapshot = vm.snapshot(); + // settled at starting price and get takerLocked back + checkSettleAsCancelled(takerId, user1); + checkWithdrawFromSettled(takerId, takerLocked); + + vm.revertTo(snapshot); + // check provider can call too + checkSettleAsCancelled(takerId, provider); + checkWithdrawFromSettled(takerId, takerLocked); + } + + function test_settleAsCancelled_NotYet() public { + (uint takerId,) = checkOpenPairedPosition(); + + // Try to settle before expiration + delay + skip(duration + takerNFT.SETTLE_AS_CANCELLED_DELAY() - 1); + startHoax(user1); + vm.expectRevert("taker: cannot be settled as cancelled yet"); + takerNFT.settleAsCancelled(takerId); + } + + function test_settleAsCancelled_AlreadySettled() public { + (uint takerId,) = checkOpenPairedPosition(); + + // Settle the position + skip(duration + takerNFT.SETTLE_AS_CANCELLED_DELAY()); + startHoax(user1); + updatePrice(); + takerNFT.settlePairedPosition(takerId); + + // Try to settle again + vm.expectRevert("taker: already settled"); + takerNFT.settleAsCancelled(takerId); + } + + function test_settleAsCancelled_AccessControl() public { + (uint takerId, uint providerId) = checkOpenPairedPosition(); + skip(duration + takerNFT.SETTLE_AS_CANCELLED_DELAY()); + + // try to settle from a different address + startHoax(address(0xdead)); + vm.expectRevert("taker: not owner of taker or provider position"); + takerNFT.settleAsCancelled(takerId); + } + function test_withdrawFromSettled_NotOwner() public { (uint takerId,) = checkOpenPairedPosition(); @@ -727,67 +820,6 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { vm.expectRevert("taker: cancel balance mismatch"); takerNFT.cancelPairedPosition(takerId); } - - function test_setOracle() public { - // new oracle - BaseTakerOracle newOracle = createMockFeedOracle(address(underlying), address(cashAsset)); - - uint newPrice = cashUnits(1500); - updatePrice(newPrice); - - startHoax(owner); - vm.expectEmit(address(takerNFT)); - emit ICollarTakerNFT.OracleSet(chainlinkOracle, newOracle); - takerNFT.setOracle(newOracle); - - assertEq(address(takerNFT.oracle()), address(newOracle)); - assertEq(takerNFT.currentOraclePrice(), newPrice); - } - - function test_revert_setOracle() public { - startHoax(user1); - vm.expectRevert("BaseManaged: not configHub owner"); - takerNFT.setOracle(chainlinkOracle); - - startHoax(owner); - - // Create an oracle with mismatched assets - BaseTakerOracle invalidOracle = createMockFeedOracle(address(cashAsset), address(underlying)); - vm.expectRevert("taker: oracle underlying mismatch"); - takerNFT.setOracle(invalidOracle); - - invalidOracle = createMockFeedOracle(address(cashAsset), address(cashAsset)); - vm.expectRevert("taker: oracle underlying mismatch"); - takerNFT.setOracle(invalidOracle); - - invalidOracle = createMockFeedOracle(address(underlying), address(underlying)); - vm.expectRevert("taker: oracle cashAsset mismatch"); - takerNFT.setOracle(invalidOracle); - - vm.mockCall(address(100), abi.encodeCall(IERC20Metadata.decimals, ()), abi.encode(18)); - invalidOracle = createMockFeedOracle(address(underlying), address(100)); - vm.expectRevert("taker: oracle cashAsset mismatch"); - takerNFT.setOracle(invalidOracle); - - BaseTakerOracle newOracle = createMockFeedOracle(address(underlying), address(cashAsset)); - vm.mockCall(address(newOracle), abi.encodeCall(ITakerOracle.currentPrice, ()), abi.encode(0)); - vm.expectRevert("taker: invalid current price"); - takerNFT.setOracle(newOracle); - vm.clearMockedCalls(); - - updatePrice(1); - // 0 conversion view - vm.mockCall(address(newOracle), abi.encodeCall(newOracle.convertToBaseAmount, (1, 1)), abi.encode(0)); - vm.expectRevert("taker: invalid convertToBaseAmount"); - takerNFT.setOracle(newOracle); - vm.clearMockedCalls(); - - // reverting conversion view - vm.mockCallRevert(address(newOracle), abi.encodeCall(newOracle.convertToBaseAmount, (1, 1)), "mocked"); - vm.expectRevert("mocked"); - takerNFT.setOracle(newOracle); - vm.clearMockedCalls(); - } } contract ReentrantAttacker { From bd06cc939bd0d9d5c26c083985c1f155f03d2489 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Thu, 3 Apr 2025 09:46:10 +1100 Subject: [PATCH 04/12] Simplifications QoL - part 1 (#140) * update view and local tests * add changelog * minor * remove pausing and ownership in basemanaged * remove pausing and ownership for rolls * remove rest of pausing and ownership, fix tests * update changelog * update version string * add version string check in fork tests * remove setOracle, add settleAsCancelled * add note re loans * remove event * remove pauseguardian from confighub, update canOpen interface * update fork test, changelog * remove special escrow auth flow * make oracle immutable --- CHANGELOG.md | 7 ++- script/libraries/ArbitrumMainnetDeployer.sol | 5 +- script/libraries/ArbitrumSepoliaDeployer.sol | 5 +- script/libraries/BaseDeployer.sol | 23 ++++--- script/libraries/OPBaseMainnetDeployer.sol | 5 +- script/libraries/OPBaseSepoliaDeployer.sol | 5 +- src/CollarProviderNFT.sol | 10 ++- src/CollarTakerNFT.sol | 8 ++- src/ConfigHub.sol | 46 +++++--------- src/EscrowSupplierNFT.sol | 22 +++---- src/LoansNFT.sol | 31 ++++++--- src/interfaces/IConfigHub.sol | 5 +- src/interfaces/IEscrowSupplierNFT.sol | 1 - .../full-protocol/AssetDeployForkTest.t.sol | 1 - .../full-protocol/BaseAssetPairForkTest.sol | 38 +++++++---- .../full-protocol/Loans.fork.arbi.t.sol | 3 - .../full-protocol/Loans.fork.opbase.t.sol | 3 - test/unit/BaseAssetPairTestSetup.sol | 10 +-- test/unit/CollarProviderNFT.t.sol | 4 +- test/unit/CollarTakerNFT.t.sol | 4 +- test/unit/ConfigHub.t.sol | 63 +++---------------- test/unit/EscrowSupplier.admin.t.sol | 37 ----------- test/unit/EscrowSupplierNFT.effects.t.sol | 2 +- test/unit/EscrowSupplierNFT.reverts.t.sol | 10 +-- test/unit/Loans.basic.effects.t.sol | 2 +- 25 files changed, 135 insertions(+), 215 deletions(-) delete mode 100644 test/unit/EscrowSupplier.admin.t.sol diff --git a/CHANGELOG.md b/CHANGELOG.md index c027fc81..fc67372e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,9 +15,14 @@ Notable changes to contracts. - `setOracle` is removed, and `settleAsCancelled` (for oracle failure) is added - CollarProviderNFT: - `protocolFee` charges fee on full notional amount (cash value of loan input), and expects `callStrikePercent` argument. +EscrowSupplierNFT: + - remove `loansCanOpen` and admin setter, and use canOpenPair on config hub for this auth instead - Rolls: - Update to use the updated protocolFee interface. - - Not inheriting from BaseManaged. + - Not inheriting from BaseManaged. +- ConfigHub + - remove pause guardians + - make canOpenPair interface more loose to allow removing supplier's `loansCanOpen` # 0.2.0 (All contracts) diff --git a/script/libraries/ArbitrumMainnetDeployer.sol b/script/libraries/ArbitrumMainnetDeployer.sol index 3a417155..6c487167 100644 --- a/script/libraries/ArbitrumMainnetDeployer.sol +++ b/script/libraries/ArbitrumMainnetDeployer.sol @@ -14,16 +14,13 @@ library ArbitrumMainnetDeployer { uint24 constant swapFeeTier = 500; function defaultHubParams() internal pure returns (BaseDeployer.HubParams memory) { - address[] memory pauseGuardians = new address[](1); - pauseGuardians[0] = Const.ArbiMain_deployerAcc; return BaseDeployer.HubParams({ minDuration: 30 days, maxDuration: 365 days, minLTV: 2500, maxLTV: 9500, feeAPR: 90, - feeRecipient: Const.ArbiMain_feeRecipient, - pauseGuardians: pauseGuardians + feeRecipient: Const.ArbiMain_feeRecipient }); } diff --git a/script/libraries/ArbitrumSepoliaDeployer.sol b/script/libraries/ArbitrumSepoliaDeployer.sol index 70be12c8..90abad56 100644 --- a/script/libraries/ArbitrumSepoliaDeployer.sol +++ b/script/libraries/ArbitrumSepoliaDeployer.sol @@ -16,16 +16,13 @@ library ArbitrumSepoliaDeployer { int constant USDSTABLEPRICE = 100_000_000; // 1 * 10^8 since feed decimals is 8 function defaultHubParams() internal pure returns (BaseDeployer.HubParams memory) { - address[] memory pauseGuardians = new address[](1); - pauseGuardians[0] = Const.ArbiSep_deployerAcc; return BaseDeployer.HubParams({ minDuration: 5 minutes, maxDuration: 365 days, minLTV: 2500, maxLTV: 9900, feeAPR: 90, - feeRecipient: Const.ArbiSep_feeRecipient, - pauseGuardians: pauseGuardians + feeRecipient: Const.ArbiSep_feeRecipient }); } diff --git a/script/libraries/BaseDeployer.sol b/script/libraries/BaseDeployer.sol index 38cf9518..63d03bc1 100644 --- a/script/libraries/BaseDeployer.sol +++ b/script/libraries/BaseDeployer.sol @@ -54,7 +54,6 @@ library BaseDeployer { uint maxDuration; address feeRecipient; uint feeAPR; - address[] pauseGuardians; } struct DeploymentResult { @@ -162,24 +161,28 @@ library BaseDeployer { } function setupContractPair(ConfigHub hub, AssetPairContracts memory pair) internal { - hub.setCanOpenPair(pair.underlying, pair.cashAsset, address(pair.takerNFT), true); - hub.setCanOpenPair(pair.underlying, pair.cashAsset, address(pair.providerNFT), true); - hub.setCanOpenPair(pair.underlying, pair.cashAsset, address(pair.loansContract), true); - hub.setCanOpenPair(pair.underlying, pair.cashAsset, address(pair.rollsContract), true); - hub.setCanOpenPair(pair.underlying, hub.ANY_ASSET(), address(pair.escrowNFT), true); + hub.setCanOpenPair(address(pair.underlying), address(pair.cashAsset), address(pair.takerNFT), true); + hub.setCanOpenPair(address(pair.underlying), address(pair.cashAsset), address(pair.providerNFT), true); + hub.setCanOpenPair( + address(pair.underlying), address(pair.cashAsset), address(pair.loansContract), true + ); + hub.setCanOpenPair( + address(pair.underlying), address(pair.cashAsset), address(pair.rollsContract), true + ); + hub.setCanOpenPair(address(pair.underlying), hub.ANY_ASSET(), address(pair.escrowNFT), true); pair.loansContract.setSwapperAllowed(address(pair.swapperUniV3), true, true); - pair.escrowNFT.setLoansCanOpen(address(pair.loansContract), true); + // allow loans to open escrow positions + hub.setCanOpenPair( + address(pair.underlying), address(pair.escrowNFT), address(pair.loansContract), true + ); } function setupConfigHub(ConfigHub configHub, HubParams memory hubParams) internal { configHub.setLTVRange(hubParams.minLTV, hubParams.maxLTV); configHub.setCollarDurationRange(hubParams.minDuration, hubParams.maxDuration); configHub.setProtocolFeeParams(hubParams.feeAPR, hubParams.feeRecipient); - for (uint i; i < hubParams.pauseGuardians.length; ++i) { - configHub.setPauseGuardian(hubParams.pauseGuardians[i], true); - } } // @dev this only nominates, and ownership must be accepted by the new owner diff --git a/script/libraries/OPBaseMainnetDeployer.sol b/script/libraries/OPBaseMainnetDeployer.sol index 3d04251e..b6ea79d5 100644 --- a/script/libraries/OPBaseMainnetDeployer.sol +++ b/script/libraries/OPBaseMainnetDeployer.sol @@ -13,16 +13,13 @@ library OPBaseMainnetDeployer { uint24 constant swapFeeTier = 500; function defaultHubParams() internal pure returns (BaseDeployer.HubParams memory) { - address[] memory pauseGuardians = new address[](1); - pauseGuardians[0] = Const.OPBaseMain_deployerAcc; return BaseDeployer.HubParams({ minDuration: 30 days, maxDuration: 365 days, minLTV: 2500, maxLTV: 9500, feeAPR: 90, - feeRecipient: Const.OPBaseMain_feeRecipient, - pauseGuardians: pauseGuardians + feeRecipient: Const.OPBaseMain_feeRecipient }); } diff --git a/script/libraries/OPBaseSepoliaDeployer.sol b/script/libraries/OPBaseSepoliaDeployer.sol index 0d5d0e7a..ed1738dd 100644 --- a/script/libraries/OPBaseSepoliaDeployer.sol +++ b/script/libraries/OPBaseSepoliaDeployer.sol @@ -16,16 +16,13 @@ library OPBaseSepoliaDeployer { int constant USDSTABLEPRICE = 100_000_000; // 1 * 10^8 since feed decimals is 8 function defaultHubParams() internal pure returns (BaseDeployer.HubParams memory) { - address[] memory pauseGuardians = new address[](1); - pauseGuardians[0] = Const.OPBaseSep_deployerAcc; return BaseDeployer.HubParams({ minDuration: 5 minutes, maxDuration: 365 days, minLTV: 2500, maxLTV: 9900, feeAPR: 90, - feeRecipient: Const.OPBaseSep_feeRecipient, - pauseGuardians: pauseGuardians + feeRecipient: Const.OPBaseSep_feeRecipient }); } diff --git a/src/CollarProviderNFT.sol b/src/CollarProviderNFT.sol index e93854c6..83a8568f 100644 --- a/src/CollarProviderNFT.sol +++ b/src/CollarProviderNFT.sol @@ -271,8 +271,14 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { { // @dev only checked on open, not checked later on settle / cancel to allow withdraw-only mode. // not checked on createOffer, so invalid offers can be created but not minted from - require(configHub.canOpenPair(underlying, cashAsset, msg.sender), "provider: unsupported taker"); - require(configHub.canOpenPair(underlying, cashAsset, address(this)), "provider: unsupported provider"); + require( + configHub.canOpenPair(address(underlying), address(cashAsset), msg.sender), + "provider: unsupported taker" + ); + require( + configHub.canOpenPair(address(underlying), address(cashAsset), address(this)), + "provider: unsupported provider" + ); LiquidityOffer memory offer = getOffer(offerId); // check terms diff --git a/src/CollarTakerNFT.sol b/src/CollarTakerNFT.sol index 03618255..0547468f 100644 --- a/src/CollarTakerNFT.sol +++ b/src/CollarTakerNFT.sol @@ -175,10 +175,14 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { returns (uint takerId, uint providerId) { // check asset & self allowed - require(configHub.canOpenPair(underlying, cashAsset, address(this)), "taker: unsupported taker"); + require( + configHub.canOpenPair(address(underlying), address(cashAsset), address(this)), + "taker: unsupported taker" + ); // check assets & provider allowed require( - configHub.canOpenPair(underlying, cashAsset, address(providerNFT)), "taker: unsupported provider" + configHub.canOpenPair(address(underlying), address(cashAsset), address(providerNFT)), + "taker: unsupported provider" ); // check assets match require(providerNFT.underlying() == underlying, "taker: underlying mismatch"); diff --git a/src/ConfigHub.sol b/src/ConfigHub.sol index 72cf3d01..b3d721eb 100644 --- a/src/ConfigHub.sol +++ b/src/ConfigHub.sol @@ -5,7 +5,7 @@ import { Ownable2Step, Ownable } from "@openzeppelin/contracts/access/Ownable2St import { EnumerableSet } from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; import { SafeCast } from "@openzeppelin/contracts/utils/math/SafeCast.sol"; -import { IConfigHub, IERC20 } from "./interfaces/IConfigHub.sol"; +import { IConfigHub } from "./interfaces/IConfigHub.sol"; /** * @title ConfigHub @@ -30,9 +30,9 @@ contract ConfigHub is Ownable2Step, IConfigHub { uint internal constant BIPS_BASE = 10_000; uint internal constant YEAR = 365 days; - string public constant VERSION = "0.2.0"; + string public constant VERSION = "0.3.0"; /// @notice placeholder value for using canOpenPair for auth when only one asset is specified - IERC20 public constant ANY_ASSET = IERC20(address(type(uint160).max)); // 0xff..ff + address public constant ANY_ASSET = address(type(uint160).max); // 0xff..ff // configuration validation (validate on set) uint public constant MAX_PROTOCOL_FEE_BIPS = BIPS_BASE / 100; // 1% @@ -51,15 +51,13 @@ contract ConfigHub is Ownable2Step, IConfigHub { uint16 public protocolFeeAPR; // uint16 max 650%, but cannot be over MAX_PROTOCOL_FEE_BIPS address public feeRecipient; // next slots - // pause guardians for other contracts - EnumerableSet.AddressSet internal pauseGuardians; /** * @notice main auth for system contracts calling each other during opening of positions: * Assets would typically be: 'underlying -> cash -> Set
', but if the auth is * for a single asset (not a pair), ANY_ASSET will be used as a placeholder for the second asset. */ - mapping(IERC20 => mapping(IERC20 => EnumerableSet.AddressSet)) internal canOpenSets; + mapping(address => mapping(address => EnumerableSet.AddressSet)) internal canOpenSets; constructor(address _initialOwner) Ownable(_initialOwner) { } @@ -77,7 +75,10 @@ contract ConfigHub is Ownable2Step, IConfigHub { * @param target The contract in the system for which the flag is being set * @param enabled Whether opening position is enabled */ - function setCanOpenPair(IERC20 assetA, IERC20 assetB, address target, bool enabled) external onlyOwner { + function setCanOpenPair(address assetA, address assetB, address target, bool enabled) + external + onlyOwner + { EnumerableSet.AddressSet storage pairSet = canOpenSets[assetA][assetB]; // contains / return value is not checked enabled ? pairSet.add(target) : pairSet.remove(target); @@ -110,16 +111,6 @@ contract ConfigHub is Ownable2Step, IConfigHub { // pausing - /// @notice Sets addresses that can pause (but not unpause) any of the contracts that - /// use this ConfigHub. - /// @param sender The address of a guardian to enable or disable - /// @param enabled Enabled or disabled status - function setPauseGuardian(address sender, bool enabled) external onlyOwner { - // contains / return value is not checked - enabled ? pauseGuardians.add(sender) : pauseGuardians.remove(sender); - emit PauseGuardianSet(sender, enabled); - } - // protocol fee /// @notice Sets the APR in BIPs, and the address that receive the protocol fee. @@ -136,19 +127,23 @@ contract ConfigHub is Ownable2Step, IConfigHub { // ----- Views ----- /// @notice main auth for system contracts calling each other during opening of positions for a pair. - function canOpenPair(IERC20 underlying, IERC20 cashAsset, address target) external view returns (bool) { + function canOpenPair(address underlying, address cashAsset, address target) + external + view + returns (bool) + { return canOpenSets[underlying][cashAsset].contains(target); } /// @notice equivalent to `canOpenPair` view when the second asset is ANY_ASSET placeholder - function canOpenSingle(IERC20 asset, address target) external view returns (bool) { + function canOpenSingle(address asset, address target) external view returns (bool) { return canOpenSets[asset][ANY_ASSET].contains(target); } /// @notice Returns all the addresses that are allowed for a pair of assets. /// @dev should be used to validate that only expected contracts are in the Set. /// assetB can be ANY_ASSET placeholder for "single asset" authorization type. - function allCanOpenPair(IERC20 assetA, IERC20 assetB) external view returns (address[] memory) { + function allCanOpenPair(address assetA, address assetB) external view returns (address[] memory) { return canOpenSets[assetA][assetB].values(); } @@ -163,15 +158,4 @@ contract ConfigHub is Ownable2Step, IConfigHub { function isValidLTV(uint ltv) external view returns (bool) { return ltv >= minLTV && ltv <= maxLTV; } - - /// @notice Checks if an address is a pause guardian: allowed to pause, but not unpause - /// contracts using ConfigHub via BaseManaged / BaseNFT - function isPauseGuardian(address sender) external view returns (bool) { - return pauseGuardians.contains(sender); - } - - /// @notice Returns all the pause guardians currently enabled - function allPauseGuardians() external view returns (address[] memory) { - return pauseGuardians.values(); - } } diff --git a/src/EscrowSupplierNFT.sol b/src/EscrowSupplierNFT.sol index fc3adcb2..05695635 100644 --- a/src/EscrowSupplierNFT.sol +++ b/src/EscrowSupplierNFT.sol @@ -61,9 +61,6 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { // @dev this is NOT the NFT id, this is a separate non transferrable ID uint public nextOfferId = 1; // starts from 1 so that 0 ID is not used - // loans contracts allowed to start or switch escrows - mapping(address loans => bool allowed) public loansCanOpen; - mapping(uint offerId => OfferStored) internal offers; mapping(uint escrowId => EscrowStored) internal escrows; @@ -265,7 +262,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { external returns (uint escrowId) { - // @dev msg.sender auth is checked vs. loansCanOpen in _startEscrow + // @dev msg.sender auth is checked vs. canOpenPair in _startEscrow escrowId = _startEscrow(offerId, escrowed, fees, loanId); // @dev despite the fact that they partially cancel out, so can be done as just fee transfer, @@ -404,24 +401,19 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { emit EscrowSeized(escrowId, msg.sender, withdrawal); } - // ----- admin ----- // - - /// @notice Sets whether a Loans contract is allowed to interact with this contract - function setLoansCanOpen(address loans, bool allowed) external onlyConfigHubOwner { - // @dev no checks for Loans interface since calls are only from Loans to this contract - loansCanOpen[loans] = allowed; - emit LoansCanOpenSet(loans, allowed); - } - // ----- INTERNAL MUTATIVE ----- // function _startEscrow(uint offerId, uint escrowed, uint fees, uint loanId) internal returns (uint escrowId) { - require(loansCanOpen[msg.sender], "escrow: unauthorized loans contract"); + // Loans should be authorized to open for underlying and this contract (as second asset) + require( + configHub.canOpenPair(address(asset), address(this), msg.sender), + "escrow: unauthorized loans contract" + ); // @dev loans is not checked since is directly authed in this contract via setLoansAllowed - require(configHub.canOpenSingle(asset, address(this)), "escrow: unsupported escrow"); + require(configHub.canOpenSingle(address(asset), address(this)), "escrow: unsupported escrow"); Offer memory offer = getOffer(offerId); require(offer.supplier != address(0), "escrow: invalid offer"); // revert here for clarity diff --git a/src/LoansNFT.sol b/src/LoansNFT.sol index 595e6b40..1211e893 100644 --- a/src/LoansNFT.sol +++ b/src/LoansNFT.sol @@ -277,7 +277,10 @@ contract LoansNFT is ILoansNFT, BaseNFT { uint newEscrowFee ) external onlyNFTOwner(loanId) returns (uint newLoanId, uint newLoanAmount, int toUser) { // check opening loans is still allowed (not in exit-only mode) - require(configHub.canOpenPair(underlying, cashAsset, address(this)), "loans: unsupported loans"); + require( + configHub.canOpenPair(address(underlying), address(cashAsset), address(this)), + "loans: unsupported loans" + ); // @dev rolls contract is assumed to not allow rolling an expired or settled position, // but checking explicitly is safer and easier to review @@ -400,7 +403,10 @@ contract LoansNFT is ILoansNFT, BaseNFT { EscrowOffer memory escrowOffer, uint escrowFees ) internal returns (uint loanId, uint providerId, uint loanAmount) { - require(configHub.canOpenPair(underlying, cashAsset, address(this)), "loans: unsupported loans"); + require( + configHub.canOpenPair(address(underlying), address(cashAsset), address(this)), + "loans: unsupported loans" + ); // taker NFT and provider NFT canOpen is checked in _swapAndMintCollar // escrow NFT canOpen is checked in _conditionalOpenEscrow @@ -461,10 +467,14 @@ contract LoansNFT is ILoansNFT, BaseNFT { ) internal returns (uint takerId, uint providerId, uint loanAmount) { (CollarProviderNFT providerNFT, uint offerId) = (offer.providerNFT, offer.id); - require(configHub.canOpenPair(underlying, cashAsset, address(takerNFT)), "loans: unsupported taker"); + require( + configHub.canOpenPair(address(underlying), address(cashAsset), address(takerNFT)), + "loans: unsupported taker" + ); // taker will check provider's canOpen as well, but we're using a view from it below so check too require( - configHub.canOpenPair(underlying, cashAsset, address(providerNFT)), "loans: unsupported provider" + configHub.canOpenPair(address(underlying), address(cashAsset), address(providerNFT)), + "loans: unsupported provider" ); // taker is expected to check that providerNFT's assets match correctly @@ -586,7 +596,10 @@ contract LoansNFT is ILoansNFT, BaseNFT { { (Rolls rolls, uint rollId) = (rollOffer.rolls, rollOffer.id); // check this rolls contract is allowed - require(configHub.canOpenPair(underlying, cashAsset, address(rolls)), "loans: unsupported rolls"); + require( + configHub.canOpenPair(address(underlying), address(cashAsset), address(rolls)), + "loans: unsupported rolls" + ); // ensure it's the right takerNFT in case of multiple takerNFTs for an asset pair require(address(rolls.takerNFT()) == address(takerNFT), "loans: rolls takerNFT mismatch"); // offer status (active) is not checked, also since rolls should check / fail @@ -641,7 +654,9 @@ contract LoansNFT is ILoansNFT, BaseNFT { // check asset matches require(escrowNFT.asset() == underlying, "loans: escrow asset mismatch"); // whitelisted only - require(configHub.canOpenSingle(underlying, address(escrowNFT)), "loans: unsupported escrow"); + require( + configHub.canOpenSingle(address(underlying), address(escrowNFT)), "loans: unsupported escrow" + ); // @dev underlyingAmount and fee were pulled already before calling this method underlying.forceApprove(address(escrowNFT), escrowed + fees); @@ -665,7 +680,9 @@ contract LoansNFT is ILoansNFT, BaseNFT { if (prevLoan.usesEscrow) { EscrowSupplierNFT escrowNFT = prevLoan.escrowNFT; // check this escrow is still allowed - require(configHub.canOpenSingle(underlying, address(escrowNFT)), "loans: unsupported escrow"); + require( + configHub.canOpenSingle(address(underlying), address(escrowNFT)), "loans: unsupported escrow" + ); underlying.safeTransferFrom(msg.sender, address(this), newFees); underlying.forceApprove(address(escrowNFT), newFees); diff --git a/src/interfaces/IConfigHub.sol b/src/interfaces/IConfigHub.sol index 4221dfd4..7c89634a 100644 --- a/src/interfaces/IConfigHub.sol +++ b/src/interfaces/IConfigHub.sol @@ -1,15 +1,12 @@ // SPDX-License-Identifier: GPL 2.0 pragma solidity 0.8.22; -import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; - interface IConfigHub { event LTVRangeSet(uint indexed minLTV, uint indexed maxLTV); event CollarDurationRangeSet(uint indexed minDuration, uint indexed maxDuration); event ContractCanOpenSet( - IERC20 indexed assetA, IERC20 indexed assetB, address indexed contractAddress, bool enabled + address indexed assetA, address indexed assetB, address indexed contractAddress, bool enabled ); - event PauseGuardianSet(address sender, bool enabled); event ProtocolFeeParamsUpdated( uint prevFeeAPR, uint newFeeAPR, address prevRecipient, address newRecipient ); diff --git a/src/interfaces/IEscrowSupplierNFT.sol b/src/interfaces/IEscrowSupplierNFT.sol index 58f01f51..c6507006 100644 --- a/src/interfaces/IEscrowSupplierNFT.sol +++ b/src/interfaces/IEscrowSupplierNFT.sol @@ -81,5 +81,4 @@ interface IEscrowSupplierNFT { event EscrowsSwitched(uint indexed oldEscrowId, uint indexed newEscrowId); event WithdrawalFromReleased(uint indexed escrowId, address indexed recipient, uint withdrawn); event EscrowSeized(uint indexed escrowId, address indexed recipient, uint withdrawn); - event LoansCanOpenSet(address indexed loansAddress, bool indexed allowed); } diff --git a/test/integration/full-protocol/AssetDeployForkTest.t.sol b/test/integration/full-protocol/AssetDeployForkTest.t.sol index 60084417..7218074a 100644 --- a/test/integration/full-protocol/AssetDeployForkTest.t.sol +++ b/test/integration/full-protocol/AssetDeployForkTest.t.sol @@ -25,7 +25,6 @@ contract AssetDeployForkTest is BaseAssetPairForkTest { function _setTestValues() internal override { protocolFeeRecipient = Const.OPBaseSep_feeRecipient; - pauseGuardians.push(Const.OPBaseSep_deployerAcc); expectedNumPairs = 1; expectedPairIndex = 0; diff --git a/test/integration/full-protocol/BaseAssetPairForkTest.sol b/test/integration/full-protocol/BaseAssetPairForkTest.sol index 2fb497ca..1d955396 100644 --- a/test/integration/full-protocol/BaseAssetPairForkTest.sol +++ b/test/integration/full-protocol/BaseAssetPairForkTest.sol @@ -35,7 +35,6 @@ abstract contract BaseAssetPairForkTest is Test { // config params uint protocolFeeAPR; address protocolFeeRecipient; - address[] pauseGuardians; // pair params uint expectedOraclePrice; @@ -351,11 +350,10 @@ abstract contract BaseAssetPairForkTest is Test { function test_validatePairDeployment() public view { // configHub - assertEq(configHub.VERSION(), "0.2.0"); + assertEq(configHub.VERSION(), "0.3.0"); assertEq(configHub.owner(), owner); assertEq(uint(configHub.protocolFeeAPR()), protocolFeeAPR); assertEq(configHub.feeRecipient(), protocolFeeRecipient); - assertEq(configHub.allPauseGuardians(), pauseGuardians); // oracle assertEq(address(pair.oracle.baseToken()), address(pair.underlying)); @@ -402,14 +400,32 @@ abstract contract BaseAssetPairForkTest is Test { assertEq(pair.escrowNFT.VERSION(), "0.3.0"); assertEq(address(pair.escrowNFT.configHubOwner()), owner); assertEq(address(pair.escrowNFT.configHub()), address(configHub)); - assertTrue(pair.escrowNFT.loansCanOpen(address(pair.loansContract))); assertEq(address(pair.escrowNFT.asset()), address(pair.underlying)); // pair auth - assertTrue(configHub.canOpenPair(pair.underlying, pair.cashAsset, address(pair.takerNFT))); - assertTrue(configHub.canOpenPair(pair.underlying, pair.cashAsset, address(pair.providerNFT))); - assertTrue(configHub.canOpenPair(pair.underlying, pair.cashAsset, address(pair.loansContract))); - assertTrue(configHub.canOpenPair(pair.underlying, pair.cashAsset, address(pair.rollsContract))); + assertTrue( + configHub.canOpenPair(address(pair.underlying), address(pair.cashAsset), address(pair.takerNFT)) + ); + assertTrue( + configHub.canOpenPair( + address(pair.underlying), address(pair.cashAsset), address(pair.providerNFT) + ) + ); + assertTrue( + configHub.canOpenPair( + address(pair.underlying), address(pair.cashAsset), address(pair.loansContract) + ) + ); + assertTrue( + configHub.canOpenPair( + address(pair.underlying), address(pair.cashAsset), address(pair.rollsContract) + ) + ); + assertTrue( + configHub.canOpenPair( + address(pair.underlying), address(pair.escrowNFT), address(pair.loansContract) + ) + ); // all pair auth address[] memory pairAuthed = new address[](4); @@ -417,15 +433,15 @@ abstract contract BaseAssetPairForkTest is Test { pairAuthed[1] = address(pair.providerNFT); pairAuthed[2] = address(pair.loansContract); pairAuthed[3] = address(pair.rollsContract); - assertEq(configHub.allCanOpenPair(pair.underlying, pair.cashAsset), pairAuthed); + assertEq(configHub.allCanOpenPair(address(pair.underlying), address(pair.cashAsset)), pairAuthed); // single asset auth - assertTrue(configHub.canOpenSingle(pair.underlying, address(pair.escrowNFT))); + assertTrue(configHub.canOpenSingle(address(pair.underlying), address(pair.escrowNFT))); // all single auth for underlying address[] memory escrowAuthed = new address[](1); escrowAuthed[0] = address(pair.escrowNFT); - assertEq(configHub.allCanOpenPair(pair.underlying, configHub.ANY_ASSET()), escrowAuthed); + assertEq(configHub.allCanOpenPair(address(pair.underlying), configHub.ANY_ASSET()), escrowAuthed); } function testAssetsSanity() public { diff --git a/test/integration/full-protocol/Loans.fork.arbi.t.sol b/test/integration/full-protocol/Loans.fork.arbi.t.sol index edd10af2..715e4798 100644 --- a/test/integration/full-protocol/Loans.fork.arbi.t.sol +++ b/test/integration/full-protocol/Loans.fork.arbi.t.sol @@ -32,7 +32,6 @@ contract WETHUSDC_ArbiMain_LoansForkTest is BaseAssetPairForkTest_ScriptTest { // config params protocolFeeAPR = 90; protocolFeeRecipient = Const.ArbiMain_feeRecipient; - pauseGuardians.push(Const.ArbiMain_deployerAcc); // @dev all pairs must be tested, so if this number is increased, test classes must be added expectedNumPairs = 3; @@ -110,8 +109,6 @@ contract WETHUSDC_ArbiSep_LoansForkTest is WETHUSDC_ArbiMain_LoansForkTest { owner = Const.ArbiSep_owner; protocolFeeRecipient = Const.ArbiSep_feeRecipient; - delete pauseGuardians; - pauseGuardians.push(Const.ArbiSep_deployerAcc); // @dev all pairs must be tested, so if this number is increased, test classes must be added expectedNumPairs = 2; diff --git a/test/integration/full-protocol/Loans.fork.opbase.t.sol b/test/integration/full-protocol/Loans.fork.opbase.t.sol index 80454da5..a822830d 100644 --- a/test/integration/full-protocol/Loans.fork.opbase.t.sol +++ b/test/integration/full-protocol/Loans.fork.opbase.t.sol @@ -88,7 +88,6 @@ contract WETHUSDC_OPBaseMain_LoansForkTest is BaseAssetPairForkTest_ScriptTest { // config params protocolFeeAPR = 90; protocolFeeRecipient = Const.OPBaseMain_feeRecipient; - pauseGuardians.push(Const.OPBaseMain_deployerAcc); // @dev all pairs must be tested, so if this number is increased, test classes must be added expectedNumPairs = 2; @@ -184,8 +183,6 @@ contract TWETHTUSDC_OPBaseSep_LoansForkTest is WETHUSDC_OPBaseMain_LoansForkTest // config params protocolFeeRecipient = Const.OPBaseSep_feeRecipient; - delete pauseGuardians; - pauseGuardians.push(Const.OPBaseSep_deployerAcc); // @dev all pairs must be tested, so if this number is increased, test classes must be added expectedNumPairs = 2; diff --git a/test/unit/BaseAssetPairTestSetup.sol b/test/unit/BaseAssetPairTestSetup.sol index 02fd824f..d8521c16 100644 --- a/test/unit/BaseAssetPairTestSetup.sol +++ b/test/unit/BaseAssetPairTestSetup.sol @@ -104,9 +104,9 @@ contract BaseAssetPairTestSetup is Test { configHub.setLTVRange(ltv, ltv); configHub.setCollarDurationRange(duration, duration); // contracts auth - configHub.setCanOpenPair(underlying, cashAsset, address(takerNFT), true); - configHub.setCanOpenPair(underlying, cashAsset, address(providerNFT), true); - configHub.setCanOpenPair(underlying, cashAsset, address(providerNFT2), true); + configHub.setCanOpenPair(address(underlying), address(cashAsset), address(takerNFT), true); + configHub.setCanOpenPair(address(underlying), address(cashAsset), address(providerNFT), true); + configHub.setCanOpenPair(address(underlying), address(cashAsset), address(providerNFT2), true); // fees configHub.setProtocolFeeParams(protocolFeeAPR, protocolFeeRecipient); @@ -115,13 +115,13 @@ contract BaseAssetPairTestSetup is Test { function setCanOpen(address target, bool enabled) internal { startHoax(owner); - configHub.setCanOpenPair(underlying, cashAsset, target, enabled); + configHub.setCanOpenPair(address(underlying), address(cashAsset), target, enabled); vm.stopPrank(); } function setCanOpenSingle(address target, bool enabled) internal { startHoax(owner); - configHub.setCanOpenPair(underlying, configHub.ANY_ASSET(), target, enabled); + configHub.setCanOpenPair(address(underlying), configHub.ANY_ASSET(), target, enabled); vm.stopPrank(); } diff --git a/test/unit/CollarProviderNFT.t.sol b/test/unit/CollarProviderNFT.t.sol index 5f9e6286..d86ba9b9 100644 --- a/test/unit/CollarProviderNFT.t.sol +++ b/test/unit/CollarProviderNFT.t.sol @@ -635,7 +635,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { providerNFT.mintFromOffer(offerId, 0, 0); // allowed for different assets, still reverts vm.startPrank(owner); - configHub.setCanOpenPair(cashAsset, underlying, takerContract, true); + configHub.setCanOpenPair(address(cashAsset), address(underlying), takerContract, true); vm.startPrank(takerContract); vm.expectRevert("provider: unsupported taker"); providerNFT.mintFromOffer(offerId, 0, 0); @@ -647,7 +647,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { providerNFT.mintFromOffer(offerId, 0, 0); // allowed for different assets, still reverts vm.startPrank(owner); - configHub.setCanOpenPair(cashAsset, underlying, address(providerNFT), true); + configHub.setCanOpenPair(address(cashAsset), address(underlying), address(providerNFT), true); vm.startPrank(takerContract); vm.expectRevert("provider: unsupported provider"); providerNFT.mintFromOffer(offerId, 0, 0); diff --git a/test/unit/CollarTakerNFT.t.sol b/test/unit/CollarTakerNFT.t.sol index 8973addf..ed375647 100644 --- a/test/unit/CollarTakerNFT.t.sol +++ b/test/unit/CollarTakerNFT.t.sol @@ -366,7 +366,7 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { takerNFT.openPairedPosition(takerLocked, providerNFT, 0); // allowed for different assets, still reverts vm.startPrank(owner); - configHub.setCanOpenPair(cashAsset, underlying, address(takerNFT), true); + configHub.setCanOpenPair(address(cashAsset), address(underlying), address(takerNFT), true); vm.startPrank(user1); vm.expectRevert("taker: unsupported taker"); takerNFT.openPairedPosition(takerLocked, providerNFT, 0); @@ -380,7 +380,7 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { takerNFT.openPairedPosition(takerLocked, providerNFT, 0); // allowed for different assets, still reverts vm.startPrank(owner); - configHub.setCanOpenPair(cashAsset, underlying, address(providerNFT), true); + configHub.setCanOpenPair(address(cashAsset), address(underlying), address(providerNFT), true); vm.startPrank(user1); vm.expectRevert("taker: unsupported provider"); takerNFT.openPairedPosition(takerLocked, providerNFT, 0); diff --git a/test/unit/ConfigHub.t.sol b/test/unit/ConfigHub.t.sol index adb924e9..00f654b5 100644 --- a/test/unit/ConfigHub.t.sol +++ b/test/unit/ConfigHub.t.sol @@ -3,14 +3,14 @@ pragma solidity 0.8.22; import "forge-std/Test.sol"; import { TestERC20 } from "../utils/TestERC20.sol"; -import { ConfigHub, IConfigHub, IERC20 } from "../../src/ConfigHub.sol"; +import { ConfigHub, IConfigHub } from "../../src/ConfigHub.sol"; import { ICollarTakerNFT } from "../../src/interfaces/ICollarTakerNFT.sol"; import { ICollarProviderNFT } from "../../src/interfaces/ICollarProviderNFT.sol"; import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; contract ConfigHubTest is Test { - TestERC20 token1; - TestERC20 token2; + address token1; + address token2; ConfigHub configHub; uint constant durationToUse = 1 days; uint constant minDurationToUse = 300; @@ -21,13 +21,12 @@ contract ConfigHubTest is Test { address router = makeAddr("router"); address owner = makeAddr("owner"); address user1 = makeAddr("user1"); - address guardian = makeAddr("guardian"); bytes user1NotAuthorized = abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, user1); function setUp() public { - token1 = new TestERC20("Test1", "TST1", 18); - token2 = new TestERC20("Test2", "TST2", 18); + token1 = address(new TestERC20("Test1", "TST1", 18)); + token2 = address(new TestERC20("Test2", "TST2", 18)); configHub = new ConfigHub(owner); } @@ -35,16 +34,13 @@ contract ConfigHubTest is Test { configHub = new ConfigHub(owner); assertEq(configHub.owner(), owner); assertEq(configHub.pendingOwner(), address(0)); - assertEq(configHub.VERSION(), "0.2.0"); - assertEq(address(configHub.ANY_ASSET()), address(type(uint160).max)); + assertEq(configHub.VERSION(), "0.3.0"); + assertEq(configHub.ANY_ASSET(), address(type(uint160).max)); assertEq(configHub.MIN_CONFIGURABLE_LTV_BIPS(), 1000); assertEq(configHub.MAX_CONFIGURABLE_LTV_BIPS(), 9999); assertEq(configHub.MIN_CONFIGURABLE_DURATION(), 300); assertEq(configHub.MAX_CONFIGURABLE_DURATION(), 5 * 365 days); assertEq(configHub.MAX_PROTOCOL_FEE_BIPS(), 100); - assertFalse(configHub.isPauseGuardian(guardian)); - assertFalse(configHub.isPauseGuardian(owner)); - assertFalse(configHub.isPauseGuardian(address(0))); assertEq(configHub.feeRecipient(), address(0)); assertEq(configHub.minDuration(), 0); assertEq(configHub.maxDuration(), 0); @@ -65,9 +61,6 @@ contract ConfigHubTest is Test { vm.expectRevert(user1NotAuthorized); configHub.setCollarDurationRange(0, 0); - vm.expectRevert(user1NotAuthorized); - configHub.setPauseGuardian(guardian, true); - vm.expectRevert(user1NotAuthorized); configHub.setProtocolFeeParams(0, address(0)); } @@ -114,7 +107,7 @@ contract ConfigHubTest is Test { // canOpenSingle vm.expectEmit(address(configHub)); - IERC20 anyAsset = configHub.ANY_ASSET(); + address anyAsset = configHub.ANY_ASSET(); emit IConfigHub.ContractCanOpenSet(token1, anyAsset, target, true); configHub.setCanOpenPair(token1, anyAsset, target, true); @@ -189,46 +182,6 @@ contract ConfigHubTest is Test { configHub.setCollarDurationRange(minDurationToUse, 10 * 365 days); } - function test_setPauseGuardian() public { - address[] memory expectedGuardians = new address[](0); - assertEq(configHub.allPauseGuardians(), expectedGuardians); - - startHoax(owner); - vm.expectEmit(address(configHub)); - emit IConfigHub.PauseGuardianSet(guardian, true); - configHub.setPauseGuardian(guardian, true); - assertTrue(configHub.isPauseGuardian(guardian)); - - expectedGuardians = new address[](1); - expectedGuardians[0] = guardian; - assertEq(configHub.allPauseGuardians(), expectedGuardians); - - // set another - vm.expectEmit(address(configHub)); - address anotherGuardian = makeAddr("anotherGuardian"); - emit IConfigHub.PauseGuardianSet(anotherGuardian, true); - configHub.setPauseGuardian(anotherGuardian, true); - assertTrue(configHub.isPauseGuardian(anotherGuardian)); - // previous is still set - assertTrue(configHub.isPauseGuardian(guardian)); - - expectedGuardians = new address[](2); - expectedGuardians[0] = guardian; - expectedGuardians[1] = anotherGuardian; - assertEq(configHub.allPauseGuardians(), expectedGuardians); - - vm.expectEmit(address(configHub)); - emit IConfigHub.PauseGuardianSet(guardian, false); - configHub.setPauseGuardian(guardian, false); - assertFalse(configHub.isPauseGuardian(guardian)); - // the other is still set - assertTrue(configHub.isPauseGuardian(anotherGuardian)); - - expectedGuardians = new address[](1); - expectedGuardians[0] = anotherGuardian; - assertEq(configHub.allPauseGuardians(), expectedGuardians); - } - function test_setProtocolFeeParams() public { startHoax(owner); diff --git a/test/unit/EscrowSupplier.admin.t.sol b/test/unit/EscrowSupplier.admin.t.sol deleted file mode 100644 index 740e631b..00000000 --- a/test/unit/EscrowSupplier.admin.t.sol +++ /dev/null @@ -1,37 +0,0 @@ -// SPDX-License-Identifier: MIT - -pragma solidity 0.8.22; - -import "forge-std/Test.sol"; -import { Pausable } from "@openzeppelin/contracts/utils/Pausable.sol"; -import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; - -import { BaseEscrowSupplierNFTTest, IEscrowSupplierNFT } from "./EscrowSupplierNFT.effects.t.sol"; - -contract EscrowSupplierNFT_AdminTest is BaseEscrowSupplierNFTTest { - function test_onlyConfigHubOwnerMethods() public { - vm.startPrank(user1); - - vm.expectRevert("BaseManaged: not configHub owner"); - escrowNFT.setLoansCanOpen(loans, true); - } - - function test_setLoansCanOpen() public { - assertEq(escrowNFT.loansCanOpen(loans), true); - - address newLoansContract = makeAddr("newLoans"); - - startHoax(owner); - vm.expectEmit(address(escrowNFT)); - emit IEscrowSupplierNFT.LoansCanOpenSet(newLoansContract, true); - escrowNFT.setLoansCanOpen(newLoansContract, true); - - assertTrue(escrowNFT.loansCanOpen(newLoansContract)); - - vm.expectEmit(address(escrowNFT)); - emit IEscrowSupplierNFT.LoansCanOpenSet(newLoansContract, false); - escrowNFT.setLoansCanOpen(newLoansContract, false); - - assertFalse(escrowNFT.loansCanOpen(newLoansContract)); - } -} diff --git a/test/unit/EscrowSupplierNFT.effects.t.sol b/test/unit/EscrowSupplierNFT.effects.t.sol index 0bb810a6..afdb539a 100644 --- a/test/unit/EscrowSupplierNFT.effects.t.sol +++ b/test/unit/EscrowSupplierNFT.effects.t.sol @@ -33,7 +33,7 @@ contract BaseEscrowSupplierNFTTest is BaseAssetPairTestSetup { setCanOpenSingle(address(escrowNFT), true); setCanOpen(loans, true); vm.startPrank(owner); - escrowNFT.setLoansCanOpen(loans, true); + configHub.setCanOpenPair(address(underlying), address(escrowNFT), address(loans), true); vm.stopPrank(); asset.mint(loans, largeUnderlying * 10); diff --git a/test/unit/EscrowSupplierNFT.reverts.t.sol b/test/unit/EscrowSupplierNFT.reverts.t.sol index eeefd501..9291d890 100644 --- a/test/unit/EscrowSupplierNFT.reverts.t.sol +++ b/test/unit/EscrowSupplierNFT.reverts.t.sol @@ -164,7 +164,7 @@ contract EscrowSupplierNFT_BasicRevertsTest is BaseEscrowSupplierNFTTest { // block loans access vm.startPrank(owner); - escrowNFT.setLoansCanOpen(loans, false); + configHub.setCanOpenPair(address(underlying), address(escrowNFT), address(loans), false); vm.startPrank(loans); vm.expectRevert("escrow: unauthorized loans contract"); escrowNFT.startEscrow(offerId, largeUnderlying / 2, escrowFee, 1000); @@ -188,7 +188,7 @@ contract EscrowSupplierNFT_BasicRevertsTest is BaseEscrowSupplierNFTTest { // block loans open vm.startPrank(owner); - escrowNFT.setLoansCanOpen(loans, false); + configHub.setCanOpenPair(address(underlying), address(escrowNFT), address(loans), false); vm.startPrank(loans); vm.expectRevert("escrow: unauthorized loans contract"); escrowNFT.switchEscrow(escrowId, newOfferId, 0, 0); @@ -204,7 +204,7 @@ contract EscrowSupplierNFT_BasicRevertsTest is BaseEscrowSupplierNFTTest { // removing loans canOpen prevents switchEscrow vm.startPrank(owner); - escrowNFT.setLoansCanOpen(loans, false); + configHub.setCanOpenPair(address(underlying), address(escrowNFT), address(loans), false); vm.startPrank(loans); vm.expectRevert("escrow: unauthorized loans contract"); escrowNFT.switchEscrow(escrowId, 0, 0, 0); @@ -217,7 +217,7 @@ contract EscrowSupplierNFT_BasicRevertsTest is BaseEscrowSupplierNFTTest { // not same loans that started vm.startPrank(owner); - escrowNFT.setLoansCanOpen(supplier1, true); + configHub.setCanOpenPair(address(underlying), address(escrowNFT), address(supplier1), false); startHoax(supplier1); vm.expectRevert("escrow: loans address mismatch"); escrowNFT.endEscrow(escrowId, 0); @@ -226,7 +226,7 @@ contract EscrowSupplierNFT_BasicRevertsTest is BaseEscrowSupplierNFTTest { // released already vm.startPrank(owner); - escrowNFT.setLoansCanOpen(loans, true); + configHub.setCanOpenPair(address(underlying), address(escrowNFT), address(loans), true); vm.startPrank(loans); asset.approve(address(escrowNFT), largeUnderlying); escrowNFT.endEscrow(escrowId, largeUnderlying / 2); diff --git a/test/unit/Loans.basic.effects.t.sol b/test/unit/Loans.basic.effects.t.sol index f43560d1..17506ffc 100644 --- a/test/unit/Loans.basic.effects.t.sol +++ b/test/unit/Loans.basic.effects.t.sol @@ -66,7 +66,7 @@ contract LoansTestBase is BaseAssetPairTestSetup { // escrow setCanOpenSingle(address(escrowNFT), true); vm.startPrank(owner); - escrowNFT.setLoansCanOpen(address(loans), true); + configHub.setCanOpenPair(address(underlying), address(escrowNFT), address(loans), true); // loans setCanOpen(address(loans), true); setCanOpen(address(rolls), true); From 97c6655800724796efc1bdff74121ebe43a332a0 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Thu, 3 Apr 2025 09:47:45 +1100 Subject: [PATCH 05/12] Follow ups for previous 0.3.0 PRs (#137-#140) (#141) * update view and local tests * add changelog * minor * remove pausing and ownership in basemanaged * remove pausing and ownership for rolls * remove rest of pausing and ownership, fix tests * update changelog * update version string * add version string check in fork tests * remove setOracle, add settleAsCancelled * add note re loans * remove event * remove pauseguardian from confighub, update canOpen interface * update fork test, changelog * remove special escrow auth flow * make oracle immutable * make settleAsCancelled open to anyone * better revert message for provider mint * add missing coverage test * update owner privileges list * add skips to NoDeploy tests * add another known issue * add previewOffer rolls view * typo * changelong add previewOffer * update CI --- .github/workflows/tests.yml | 1 - CHANGELOG.md | 13 +++--- devdocs/audits.briefing.md | 31 +++++++------- src/CollarProviderNFT.sol | 4 +- src/CollarTakerNFT.sol | 15 ++----- src/Rolls.sol | 16 ++++++-- .../full-protocol/Loans.fork.arbi.t.sol | 2 + .../full-protocol/Loans.fork.opbase.t.sol | 8 ++++ test/unit/CollarProviderNFT.t.sol | 2 +- test/unit/CollarTakerNFT.t.sol | 40 ++++++++++++++++--- test/unit/Loans.basic.effects.t.sol | 21 ++++++++++ test/unit/Loans.rolls.reverts.t.sol | 2 +- test/unit/Rolls.t.sol | 4 ++ 13 files changed, 111 insertions(+), 48 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a7cb18d3..d952d991 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -3,7 +3,6 @@ on: push: branches: [ main, staging, develop ] pull_request: - branches: [ main, staging, develop ] concurrency: group: ${{ github.workflow }}-${{ github.ref }} diff --git a/CHANGELOG.md b/CHANGELOG.md index fc67372e..1a81d506 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,23 +4,24 @@ Notable changes to contracts. # Unreleased (0.3.0) -- BaseManaged: +BaseManaged: - No direct ownership, instead has `onlyConfigHubOwner` modifier. - All pausing removed. -- CollarTakerNFT, LoansNFT, EscrowSupplierNFT, CollarProviderNFT: +CollarTakerNFT, LoansNFT, EscrowSupplierNFT, CollarProviderNFT: - Inheriting from updated BaseManaged (not ownable, not pausable). - Different constructor interface. - Using `onlyConfigHubOwner` for admin methods. -- CollarTakerNFT: +CollarTakerNFT: - `setOracle` is removed, and `settleAsCancelled` (for oracle failure) is added -- CollarProviderNFT: +CollarProviderNFT: - `protocolFee` charges fee on full notional amount (cash value of loan input), and expects `callStrikePercent` argument. EscrowSupplierNFT: - remove `loansCanOpen` and admin setter, and use canOpenPair on config hub for this auth instead -- Rolls: +Rolls: - Update to use the updated protocolFee interface. - Not inheriting from BaseManaged. -- ConfigHub + - Add `previewOffer` view +ConfigHub - remove pause guardians - make canOpenPair interface more loose to allow removing supplier's `loansCanOpen` diff --git a/devdocs/audits.briefing.md b/devdocs/audits.briefing.md index b17148e9..2a9b92b9 100644 --- a/devdocs/audits.briefing.md +++ b/devdocs/audits.briefing.md @@ -4,20 +4,20 @@ ------------------------------------------------------------------------------------------ File blank comment code ------------------------------------------------------------------------------------------ -src/LoansNFT.sol 108 380 364 -src/EscrowSupplierNFT.sol 75 230 270 -src/CollarTakerNFT.sol 57 152 196 -src/Rolls.sol 54 205 196 -src/CollarProviderNFT.sol 48 153 188 -src/ConfigHub.sol 25 73 79 +src/LoansNFT.sol 108 380 372 +src/EscrowSupplierNFT.sol 72 227 262 +src/CollarTakerNFT.sol 57 175 217 +src/CollarProviderNFT.sol 55 176 199 +src/Rolls.sol 54 204 193 +src/ConfigHub.sol 22 64 75 src/CombinedOracle.sol 14 40 56 src/ChainlinkOracle.sol 14 50 55 src/SwapperUniV3.sol 9 53 43 -src/base/BaseManaged.sol 17 36 43 +src/base/BaseManaged.sol 17 44 39 src/base/BaseTakerOracle.sol 14 57 34 -src/base/BaseNFT.sol 8 13 24 +src/base/BaseNFT.sol 7 9 16 ------------------------------------------------------------------------------------------ -SUM: 443 1442 1548 +SUM: 443 1479 1561 ------------------------------------------------------------------------------------------ ``` @@ -30,7 +30,7 @@ SUM: 443 1442 1548 - (for checklist use: check changes to https://github.com/d-xo/weird-erc20) ## Deployment Destinations -Base and Arbitrum initially. +Base, possibly Eth L1 later. ## Other Documentation - Solidity files comments contain the most up to date documentation @@ -40,8 +40,10 @@ Base and Arbitrum initially. ## Known Issues - Providers offers do not limit execution price (only strike percentages), nor have deadlines, and are expected to be actively managed. - No refund of protocol fee for position cancellations / rolls. Fee APR and roll frequency are assumed to be low, and rolls are assumed to be beneficial enough to users to be worth it. Accepted as low risk economic issue. +- Protocol fee (charged from provider offer, on top of provider position) can be high relative to provider position's size, especially for smaller callStrikePercent. - Because oracle prices undergo multiple conversions (feeds, tokens units), asset and price feed combinations w.r.t to decimals and price ranges (e.g., low price tokens) are assumed to be checked to allow sufficient precision. - In case of congestion, calls for `openPairedPosition` (`openLoan` that uses it), and rolls `executeRoll` can be executed at higher price than the user intended (if price is lower, `openLoan` and `executeRoll` have slippage protection, and `openPairedPosition` has better upside for the caller). This is accepted as low likelihood, and low impact: loss is small since short congestion will result in small price change vs. original intent, and long downtime may fail the oracle sequencer uptime check. +- If an oracle becomes malicious, there isn't a way to "unset" it. ConfigHub can prevent opening new positions for that pair, but existing positions will remain vulnerable. - Issues and considerations explained in the Solidity comments and audit reports. ## Commonly Noted Non-issues (unless we're wrong, and they are) @@ -58,13 +60,10 @@ Base and Arbitrum initially. - `minDuration` is at least 1 month. - `maxLTV` is reasonably far from 99.99% -## Owner privileges (for ownable contracts) -- Can rescue tokens using `rescueTokens` **except** for the main asset of each contract: cannot rescue the cash asset from providerNFT or takerNFT, cannot rescue underlying from escrowNFT, cannot rescue takerNFT from loansNFT. Any asset can be rescued from Rolls. -- Can pause and unpause all non-NFT user methods, including withdrawals, on each contract separately. -- Can update the oracle address on the takerNFT. +## ConfigHub's owner privileges (for BaseManaged contracts) +- Can rescue tokens using `rescueTokens` **except** for the main asset of each BaseManaged contract: cannot rescue the cash asset from providerNFT or takerNFT, cannot rescue underlying from escrowNFT, cannot rescue takerNFT from loansNFT. - Can update the loansNFT closing keeper address and allowed swappers. -- Can update the values set in ConfigHub and replace the ConfigHub contract that's being used. This includes LTV range, durations range, protocol fee parameters, pause guardian addresses. -- Can set what internal contracts are allowed to open positions (primarily via the ConfigHub). +- Can update the values set in ConfigHub and replace the ConfigHub contract that's being used. This includes what internal contracts are allowed to open positions, LTV range, durations range, protocol fee parameters. ## Testing and POC - Install run tests excluding fork tests: `forge install && forge build && forge test --nmc Fork` diff --git a/src/CollarProviderNFT.sol b/src/CollarProviderNFT.sol index 83a8568f..abc18f93 100644 --- a/src/CollarProviderNFT.sol +++ b/src/CollarProviderNFT.sol @@ -129,7 +129,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { * @notice Calculates the protocol fee charged from offers on position creation. * The fee is charged on the full notional value of the underlying's cash value - i.e. on 100%. * Example: If providerLocked is 100, and callsTrike is 110%, the notional is 1000. If the APR is 1%, - * so the fee will on 1 year duration will be 1% of 1000 = 10, **on top** of the providerLocked. + * so the fee on 1 year duration will be 1% of 1000 = 10, **on top** of the providerLocked. * So when position is created, the offer amount will be reduced by 100 + 10 in this example, * with 100 in providerLocked, and 10 sent to protocol fee recipient. * @dev fee is set to 0 if recipient is zero because no transfer will be done @@ -293,7 +293,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { // check amount require(providerLocked >= offer.minLocked, "provider: amount too low"); uint prevOfferAmount = offer.available; - require(providerLocked + fee <= prevOfferAmount, "provider: amount too high"); + require(providerLocked + fee <= prevOfferAmount, "provider: offer < position + fee"); uint newAvailable = prevOfferAmount - providerLocked - fee; // storage updates diff --git a/src/CollarTakerNFT.sol b/src/CollarTakerNFT.sol index 0547468f..0ad18b7a 100644 --- a/src/CollarTakerNFT.sol +++ b/src/CollarTakerNFT.sol @@ -280,11 +280,9 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { * * @dev this method exists because if the oracle starts reverting, there is no way to update it. * Either taker or provider are incentivised to call settlePairedPosition prior to this method being callable, - * and anyone else (like a keeper), can call it too. So if it wasn't called by anyone, we need to settle the - * positions without an oracle, and allow their withdrawal. - * Only callable by a holder of either the taker or provider positions. If the NFT is held - * by another contract it needs to allow withdrawing the NFT to call this method. For example - * LoansNFT.unwrapAndCancelLoan may need to be used in Loans by the borrower to call this method. + * and anyone else (like a keeper), can call settlePairedPosition too. + * So if it wasn't called by anyone, we need to settle the positions without an oracle, and allow their withdrawal. + * Callable by anyone for consistency with settlePairedPosition and to allow loan closure without unwrapping. */ function settleAsCancelled(uint takerId) external nonReentrant { // @dev this checks position exists @@ -297,13 +295,6 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { ); require(!position.settled, "taker: already settled"); - // must be taker or provider NFT owner to call this. ownerOf can revert if NFT is burned, - // but that can only happen after successful settlement. - require( - msg.sender == ownerOf(takerId) || msg.sender == providerNFT.ownerOf(providerId), - "taker: not owner of taker or provider position" - ); - // store changes positions[takerId].settled = true; positions[takerId].withdrawable = position.takerLocked; diff --git a/src/Rolls.sol b/src/Rolls.sol index bedf2518..d7731135 100644 --- a/src/Rolls.sol +++ b/src/Rolls.sol @@ -129,9 +129,19 @@ contract Rolls is IRolls { * @return PreviewResults Struct of preview results calculations */ function previewRoll(uint rollId, uint price) external view returns (PreviewResults memory) { - RollOffer memory offer = getRollOffer(rollId); - // do not divide by zero - require(offer.feeReferencePrice != 0, "rolls: invalid rollId"); + return previewOffer(getRollOffer(rollId), price); + } + + /** + * @notice Calculates the amounts to be transferred for a roll offer at a specific price. + * Can be used off-chain for calculating offer parameters (like minToProvider) in the UI. + * Does not check any validity conditions (existence, deadline, price range, etc...). + * @param offer a struct of roll offer parameters to use + * @param price The price to use for the calculation + * @return PreviewResults Struct of preview results calculations + */ + function previewOffer(RollOffer memory offer, uint price) public view returns (PreviewResults memory) { + require(offer.feeReferencePrice != 0, "rolls: invalid roll offer"); // do not divide by zero int rollFee = calculateRollFee(offer, price); return _previewRoll(takerNFT.getPosition(offer.takerId), price, rollFee); } diff --git a/test/integration/full-protocol/Loans.fork.arbi.t.sol b/test/integration/full-protocol/Loans.fork.arbi.t.sol index 715e4798..e1d12e62 100644 --- a/test/integration/full-protocol/Loans.fork.arbi.t.sol +++ b/test/integration/full-protocol/Loans.fork.arbi.t.sol @@ -178,6 +178,7 @@ contract WETHUSDC_ArbiSep_LoansForkTest_NoDeploy is ArbiSep_LoansForkTest_Latest function _setTestValues() internal virtual override { super._setTestValues(); testMode = TestMode.noDeploy; + vm.skip(true); // deployment is outdated (0.2.0), ArbiSep isn't planned to be updated realArtifactsName = Const.ArbiSep_artifactsName; } } @@ -186,6 +187,7 @@ contract WBTCUSDC_ArbiSep_LoansForkTest_NoDeploy is WBTCUSDC_ArbiSep_LoansForkTe function _setTestValues() internal virtual override { super._setTestValues(); testMode = TestMode.noDeploy; + vm.skip(true); // deployment is outdated (0.2.0), ArbiSep isn't planned to be updated realArtifactsName = Const.ArbiSep_artifactsName; } } diff --git a/test/integration/full-protocol/Loans.fork.opbase.t.sol b/test/integration/full-protocol/Loans.fork.opbase.t.sol index a822830d..0f107cc3 100644 --- a/test/integration/full-protocol/Loans.fork.opbase.t.sol +++ b/test/integration/full-protocol/Loans.fork.opbase.t.sol @@ -126,6 +126,8 @@ contract WETHUSDC_OPBaseMain_LoansForkTest_NoDeploy is WETHUSDC_OPBaseMain_Loans super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; + // TODO: enable test when new release is deployed + vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseMain_artifactsName; } } @@ -155,6 +157,8 @@ contract CBBTCUSDC_OPBaseMain_LoansForkTest_NoDeploy is CBBTCUSDC_OPBaseMain_Loa super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; + // TODO: enable test when new release is deployed + vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseMain_artifactsName; } } @@ -202,6 +206,8 @@ contract TWETHTUSDC_OPBaseSep_LoansForkTest_NoDeploy is TWETHTUSDC_OPBaseSep_Loa super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; + // TODO: enable test when new release is deployed + vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseSep_artifactsName; } } @@ -227,6 +233,8 @@ contract TWBTCTUSDC_OPBaseSep_LoansForkTest_NoDeploy is TWBTCTUSDC_OPBaseSep_Loa super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; + // TODO: enable test when new release is deployed + vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseSep_artifactsName; } } diff --git a/test/unit/CollarProviderNFT.t.sol b/test/unit/CollarProviderNFT.t.sol index d86ba9b9..9404d6a0 100644 --- a/test/unit/CollarProviderNFT.t.sol +++ b/test/unit/CollarProviderNFT.t.sol @@ -680,7 +680,7 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { (uint offerId,) = createAndCheckOffer(provider, largeCash); vm.startPrank(address(takerContract)); - vm.expectRevert("provider: amount too high"); + vm.expectRevert("provider: offer < position + fee"); providerNFT.mintFromOffer(offerId, largeCash + 1, 0); } diff --git a/test/unit/CollarTakerNFT.t.sol b/test/unit/CollarTakerNFT.t.sol index ed375647..b93840ec 100644 --- a/test/unit/CollarTakerNFT.t.sol +++ b/test/unit/CollarTakerNFT.t.sol @@ -651,15 +651,20 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { vm.expectRevert(new bytes(0)); takerNFT.currentOraclePrice(); - uint snapshot = vm.snapshot(); + uint snapshot = vm.snapshotState(); // settled at starting price and get takerLocked back checkSettleAsCancelled(takerId, user1); checkWithdrawFromSettled(takerId, takerLocked); - vm.revertTo(snapshot); + vm.revertToState(snapshot); // check provider can call too checkSettleAsCancelled(takerId, provider); checkWithdrawFromSettled(takerId, takerLocked); + + vm.revertToState(snapshot); + // check someone else can call too + checkSettleAsCancelled(takerId, keeper); + checkWithdrawFromSettled(takerId, takerLocked); } function test_settleAsCancelled_NotYet() public { @@ -686,13 +691,24 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { takerNFT.settleAsCancelled(takerId); } - function test_settleAsCancelled_AccessControl() public { + function test_settleAsCancelled_balanceMismatch() public { (uint takerId, uint providerId) = checkOpenPairedPosition(); skip(duration + takerNFT.SETTLE_AS_CANCELLED_DELAY()); + DonatingProvider donatingProvider = new DonatingProvider(cashAsset); + deal(address(cashAsset), address(donatingProvider), 1); + // also mock getPosition with actual's getPosition data because it's called inside settleAsCancelled + // for some reason this needs to be mocked before the call to etch (doesn't work the other way around) + vm.mockCall( + address(providerNFT), + abi.encodeCall(providerNFT.getPosition, (providerId)), + abi.encode(providerNFT.getPosition(providerId)) + ); + // switch implementation to one that sends funds + vm.etch(address(providerNFT), address(donatingProvider).code); - // try to settle from a different address - startHoax(address(0xdead)); - vm.expectRevert("taker: not owner of taker or provider position"); + vm.startPrank(user1); + // try to settle + vm.expectRevert("taker: settle balance mismatch"); takerNFT.settleAsCancelled(takerId); } @@ -841,3 +857,15 @@ contract ReentrantAttacker { } } } + +contract DonatingProvider { + TestERC20 public immutable cashAsset; + + constructor(TestERC20 _cashAsset) { + cashAsset = _cashAsset; + } + + function settlePosition(uint, int) external { + cashAsset.transfer(msg.sender, 1); + } +} diff --git a/test/unit/Loans.basic.effects.t.sol b/test/unit/Loans.basic.effects.t.sol index 17506ffc..3310abb4 100644 --- a/test/unit/Loans.basic.effects.t.sol +++ b/test/unit/Loans.basic.effects.t.sol @@ -574,6 +574,27 @@ contract LoansBasicEffectsTest is LoansTestBase { closeAndCheckLoan(loanId, user1, loanAmount, withdrawal, swapOut); } + function test_closeLoan_settledAsCancelled() public { + (uint loanId,, uint loanAmount) = createAndCheckLoan(); + skip(duration + takerNFT.SETTLE_AS_CANCELLED_DELAY()); + + // oracle reverts now + vm.mockCallRevert(address(takerNFT.oracle()), abi.encodeCall(takerNFT.oracle().currentPrice, ()), ""); + // check that settlement reverts + vm.expectRevert(new bytes(0)); + takerNFT.settlePairedPosition(loanId); + + // settle as cancelled works + takerNFT.settleAsCancelled(loanId); + + // withdrawal: no price settlement change since settled as cancelled + uint withdrawal = takerNFT.getPosition({ takerId: loanId }).takerLocked; + // setup router output + uint swapOut = prepareDefaultSwapToUnderlying(); + // closing works + closeAndCheckLoan(loanId, user1, loanAmount, withdrawal, swapOut); + } + function test_closeLoan_byKeeper() public { (uint loanId,, uint loanAmount) = createAndCheckLoan(); skip(duration); diff --git a/test/unit/Loans.rolls.reverts.t.sol b/test/unit/Loans.rolls.reverts.t.sol index e193b742..dfe17f78 100644 --- a/test/unit/Loans.rolls.reverts.t.sol +++ b/test/unit/Loans.rolls.reverts.t.sol @@ -43,7 +43,7 @@ contract LoansRollsRevertsTest is LoansRollTestBase { // Non-existent roll offer vm.startPrank(user1); - vm.expectRevert("rolls: invalid rollId"); + vm.expectRevert("rolls: invalid roll offer"); loans.rollLoan(loanId, rollOffer(rollId + 1), MIN_INT, 0, 0); // cancelled roll offer diff --git a/test/unit/Rolls.t.sol b/test/unit/Rolls.t.sol index 96230328..b24d6557 100644 --- a/test/unit/Rolls.t.sol +++ b/test/unit/Rolls.t.sol @@ -154,6 +154,10 @@ contract RollsTest is BaseAssetPairTestSetup { assertEq(preview.newTakerLocked, expected.newTakerLocked); assertEq(preview.newProviderLocked, expected.newProviderLocked); assertEq(preview.protocolFee, expected.toProtocol); + + // check both views are equivalent + IRolls.PreviewResults memory preview2 = rolls.previewOffer(rolls.getRollOffer(rollId), newPrice); + assertEq(abi.encode(preview), abi.encode(preview2)); } // stack too deep From 41ff76fd5154894cd05eab5d4ba9dbb96b9743d8 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Thu, 3 Apr 2025 09:53:19 +1100 Subject: [PATCH 06/12] Remove rescueTokens (#142) * update view and local tests * add changelog * minor * remove pausing and ownership in basemanaged * remove pausing and ownership for rolls * remove rest of pausing and ownership, fix tests * update changelog * update version string * add version string check in fork tests * remove setOracle, add settleAsCancelled * add note re loans * remove event * remove pauseguardian from confighub, update canOpen interface * update fork test, changelog * remove special escrow auth flow * make oracle immutable * make settleAsCancelled open to anyone * better revert message for provider mint * add missing coverage test * update owner privileges list * add skips to NoDeploy tests * add another known issue * add previewOffer rolls view * typo * changelong add previewOffer * update CI * remove rescueTokens --- CHANGELOG.md | 3 +- devdocs/audits.briefing.md | 12 +-- src/CollarProviderNFT.sol | 2 +- src/CollarTakerNFT.sol | 2 +- src/EscrowSupplierNFT.sol | 2 +- src/LoansNFT.sol | 2 +- src/base/BaseManaged.sol | 45 +---------- src/base/BaseNFT.sol | 4 +- test/unit/BaseManaged.all.t.sol | 96 ++++------------------- test/unit/CollarProviderNFT.t.sol | 1 - test/unit/CollarTakerNFT.t.sol | 1 - test/unit/EscrowSupplierNFT.effects.t.sol | 1 - test/unit/Loans.basic.effects.t.sol | 1 - 13 files changed, 31 insertions(+), 141 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a81d506..f0b93f54 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Notable changes to contracts. BaseManaged: - No direct ownership, instead has `onlyConfigHubOwner` modifier. + - `rescueTokens` removed. - All pausing removed. CollarTakerNFT, LoansNFT, EscrowSupplierNFT, CollarProviderNFT: - Inheriting from updated BaseManaged (not ownable, not pausable). @@ -16,7 +17,7 @@ CollarTakerNFT: CollarProviderNFT: - `protocolFee` charges fee on full notional amount (cash value of loan input), and expects `callStrikePercent` argument. EscrowSupplierNFT: - - remove `loansCanOpen` and admin setter, and use canOpenPair on config hub for this auth instead + - remove `loansCanOpen` and admin setter, and use canOpenPair on configHub for this auth instead Rolls: - Update to use the updated protocolFee interface. - Not inheriting from BaseManaged. diff --git a/devdocs/audits.briefing.md b/devdocs/audits.briefing.md index 2a9b92b9..5a8e275d 100644 --- a/devdocs/audits.briefing.md +++ b/devdocs/audits.briefing.md @@ -1,4 +1,4 @@ -## 2024 Jan Scope: +## 2025 Apr Scope: ``` ------------------------------------------------------------------------------------------ @@ -8,16 +8,16 @@ src/LoansNFT.sol 108 380 372 src/EscrowSupplierNFT.sol 72 227 262 src/CollarTakerNFT.sol 57 175 217 src/CollarProviderNFT.sol 55 176 199 -src/Rolls.sol 54 204 193 +src/Rolls.sol 55 211 195 src/ConfigHub.sol 22 64 75 src/CombinedOracle.sol 14 40 56 src/ChainlinkOracle.sol 14 50 55 src/SwapperUniV3.sol 9 53 43 -src/base/BaseManaged.sol 17 44 39 src/base/BaseTakerOracle.sol 14 57 34 +src/base/BaseManaged.sol 14 22 25 src/base/BaseNFT.sol 7 9 16 ------------------------------------------------------------------------------------------ -SUM: 443 1479 1561 +SUM: 441 1464 1549 ------------------------------------------------------------------------------------------ ``` @@ -44,6 +44,7 @@ Base, possibly Eth L1 later. - Because oracle prices undergo multiple conversions (feeds, tokens units), asset and price feed combinations w.r.t to decimals and price ranges (e.g., low price tokens) are assumed to be checked to allow sufficient precision. - In case of congestion, calls for `openPairedPosition` (`openLoan` that uses it), and rolls `executeRoll` can be executed at higher price than the user intended (if price is lower, `openLoan` and `executeRoll` have slippage protection, and `openPairedPosition` has better upside for the caller). This is accepted as low likelihood, and low impact: loss is small since short congestion will result in small price change vs. original intent, and long downtime may fail the oracle sequencer uptime check. - If an oracle becomes malicious, there isn't a way to "unset" it. ConfigHub can prevent opening new positions for that pair, but existing positions will remain vulnerable. +- Any tokens accidentally sent to any of the contracts cannot be rescued. - Issues and considerations explained in the Solidity comments and audit reports. ## Commonly Noted Non-issues (unless we're wrong, and they are) @@ -61,9 +62,8 @@ Base, possibly Eth L1 later. - `maxLTV` is reasonably far from 99.99% ## ConfigHub's owner privileges (for BaseManaged contracts) -- Can rescue tokens using `rescueTokens` **except** for the main asset of each BaseManaged contract: cannot rescue the cash asset from providerNFT or takerNFT, cannot rescue underlying from escrowNFT, cannot rescue takerNFT from loansNFT. -- Can update the loansNFT closing keeper address and allowed swappers. - Can update the values set in ConfigHub and replace the ConfigHub contract that's being used. This includes what internal contracts are allowed to open positions, LTV range, durations range, protocol fee parameters. +- Can update the loansNFT closing keeper address and allowed swappers. ## Testing and POC - Install run tests excluding fork tests: `forge install && forge build && forge test --nmc Fork` diff --git a/src/CollarProviderNFT.sol b/src/CollarProviderNFT.sol index abc18f93..582a482e 100644 --- a/src/CollarProviderNFT.sol +++ b/src/CollarProviderNFT.sol @@ -71,7 +71,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { address _taker, string memory _name, string memory _symbol - ) BaseNFT(_name, _symbol, _configHub, address(_cashAsset)) { + ) BaseNFT(_name, _symbol, _configHub) { cashAsset = _cashAsset; underlying = _underlying; taker = _taker; diff --git a/src/CollarTakerNFT.sol b/src/CollarTakerNFT.sol index 0ad18b7a..74cdefd0 100644 --- a/src/CollarTakerNFT.sol +++ b/src/CollarTakerNFT.sol @@ -67,7 +67,7 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { ITakerOracle _oracle, string memory _name, string memory _symbol - ) BaseNFT(_name, _symbol, _configHub, address(_cashAsset)) { + ) BaseNFT(_name, _symbol, _configHub) { cashAsset = _cashAsset; underlying = _underlying; _checkOracle(_oracle); diff --git a/src/EscrowSupplierNFT.sol b/src/EscrowSupplierNFT.sol index 05695635..e0e1914b 100644 --- a/src/EscrowSupplierNFT.sol +++ b/src/EscrowSupplierNFT.sol @@ -66,7 +66,7 @@ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { mapping(uint escrowId => EscrowStored) internal escrows; constructor(ConfigHub _configHub, IERC20 _asset, string memory _name, string memory _symbol) - BaseNFT(_name, _symbol, _configHub, address(_asset)) + BaseNFT(_name, _symbol, _configHub) { asset = _asset; } diff --git a/src/LoansNFT.sol b/src/LoansNFT.sol index 1211e893..2cba9b6c 100644 --- a/src/LoansNFT.sol +++ b/src/LoansNFT.sol @@ -74,7 +74,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { address public defaultSwapper; constructor(CollarTakerNFT _takerNFT, string memory _name, string memory _symbol) - BaseNFT(_name, _symbol, _takerNFT.configHub(), address(_takerNFT)) + BaseNFT(_name, _symbol, _takerNFT.configHub()) { takerNFT = _takerNFT; cashAsset = _takerNFT.cashAsset(); diff --git a/src/base/BaseManaged.sol b/src/base/BaseManaged.sol index 99364c3c..51cda3f3 100644 --- a/src/base/BaseManaged.sol +++ b/src/base/BaseManaged.sol @@ -1,9 +1,6 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity 0.8.22; -import { SafeERC20, IERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; -import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; - import { ConfigHub } from "../ConfigHub.sol"; /** @@ -11,33 +8,18 @@ import { ConfigHub } from "../ConfigHub.sol"; * @notice Base contract for managed contracts in the protocol * @dev This contract provides common functionality for contracts that are managed * via the a common ConfigHub. It includes a onlyConfigHubOwner that can control access to - * configuration value setters in inheriting contracts, and a function to rescue stuck tokens. - * @dev ConfigHub should be owned by a secure multi-sig due the sensitive onlyConfigHubOwner methods. + * configuration value setters in inheriting contracts, and a function to update the configHub address. + * @dev ConfigHub should be owned by a secure multi-sig due its values being used in inheriting contracts. */ abstract contract BaseManaged { // ----- State ----- // ConfigHub public configHub; - /** - * @notice a token address (ERC-20 or ERC-721) that configHub's owner can NOT rescue via `rescueTokens`. - * This should be the main asset that the contract holds at rest on behalf of its users. - * If this asset is sent to the contract by mistake, there is no way to rescue it. - * This exclusion reduces the centralisation risk, and reduces the impact, incentive, and - * thus also the likelihood of compromizing the configHub's owner. - * @dev this is a single address because currently only a single asset is held by each contract - * at rest (except for Rolls). - * Other assets, that may be held by the contract in-transit (during a transaction), can be rescued. - * If more assets need to be unrescuable, this should become a Set. - */ - address public immutable unrescuableAsset; - // ----- Events ----- // event ConfigHubUpdated(ConfigHub previousConfigHub, ConfigHub newConfigHub); - event TokensRescued(address tokenContract, uint amountOrId); - constructor(ConfigHub _configHub, address _unrescuableAsset) { + constructor(ConfigHub _configHub) { _setConfigHub(_configHub); - unrescuableAsset = _unrescuableAsset; } // ----- Modifiers ----- // @@ -67,27 +49,6 @@ abstract contract BaseManaged { _setConfigHub(_newConfigHub); } - /** - * @notice Sends an amount of an ERC-20, or an ID of any ERC-721, to configHub owner's address. - * Reverts if the `token` matches the `unrescuableAsset`. - * To be used to rescue stuck assets that were sent to the contract by mistake. - * @dev Only callable by the configHub's owner. - * @param token The address of the token contract - * @param amountOrId The amount of tokens to rescue (or token ID for NFTs) - * @param isNFT Whether the token is an NFT (true) or an ERC-20 (false) - */ - function rescueTokens(address token, uint amountOrId, bool isNFT) external onlyConfigHubOwner { - require(token != unrescuableAsset, "unrescuable asset"); - if (isNFT) { - IERC721(token).transferFrom(address(this), configHubOwner(), amountOrId); - } else { - // for ERC-20 must use transfer, since most implementation won't allow transferFrom - // without approve (even from owner) - SafeERC20.safeTransfer(IERC20(token), configHubOwner(), amountOrId); - } - emit TokensRescued(token, amountOrId); - } - // ----- INTERNAL MUTATIVE ----- // function _setConfigHub(ConfigHub _newConfigHub) internal { diff --git a/src/base/BaseNFT.sol b/src/base/BaseNFT.sol index 370f6823..5cc4ff52 100644 --- a/src/base/BaseNFT.sol +++ b/src/base/BaseNFT.sol @@ -16,8 +16,8 @@ abstract contract BaseNFT is BaseManaged, ERC721 { // ----- State ----- // uint internal nextTokenId = 1; // NFT token ID, starts from 1 so that 0 ID is not used - constructor(string memory _name, string memory _symbol, ConfigHub _configHub, address _unrescuableAsset) - BaseManaged(_configHub, _unrescuableAsset) + constructor(string memory _name, string memory _symbol, ConfigHub _configHub) + BaseManaged(_configHub) ERC721(_name, _symbol) { } diff --git a/test/unit/BaseManaged.all.t.sol b/test/unit/BaseManaged.all.t.sol index 987edb03..84460a3a 100644 --- a/test/unit/BaseManaged.all.t.sol +++ b/test/unit/BaseManaged.all.t.sol @@ -4,8 +4,6 @@ pragma solidity 0.8.22; import "forge-std/Test.sol"; -import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; - import { TestERC20 } from "../utils/TestERC20.sol"; import { MockChainlinkFeed } from "../utils/MockChainlinkFeed.sol"; @@ -17,20 +15,9 @@ import { EscrowSupplierNFT } from "../../src/EscrowSupplierNFT.sol"; import { LoansNFT } from "../../src/LoansNFT.sol"; import { ChainlinkOracle } from "../../src/ChainlinkOracle.sol"; -contract TestERC721 is ERC721 { - constructor() ERC721("TestERC721", "TestERC721") { } - - function mint(address to, uint id) external { - _mint(to, id); - } -} - // base contract for other tests that will check this functionality abstract contract BaseManagedTestBase is Test { - TestERC20 erc20Rescuable; - TestERC721 erc721Rescuable; ConfigHub configHub; - address unrescuable; BaseManaged testedContract; @@ -38,13 +25,10 @@ abstract contract BaseManagedTestBase is Test { address user1 = makeAddr("user1"); function setUp() public virtual { - erc20Rescuable = new TestERC20("TestERC20", "TestERC20", 18); - erc721Rescuable = new TestERC721(); configHub = new ConfigHub(owner); setupTestedContract(); - vm.label(address(erc20Rescuable), "TestERC20"); vm.label(address(configHub), "ConfigHub"); } @@ -55,7 +39,6 @@ abstract contract BaseManagedTestBase is Test { setupTestedContract(); assertEq(testedContract.configHubOwner(), owner); assertEq(address(testedContract.configHub()), address(configHub)); - assertEq(address(testedContract.unrescuableAsset()), unrescuable); } function test_setConfigHub() public { @@ -69,33 +52,6 @@ abstract contract BaseManagedTestBase is Test { assertEq(address(testedContract.configHub()), address(newConfigHub)); } - function test_rescueTokens_ERC20() public { - uint amount = 1000; - erc20Rescuable.mint(address(testedContract), amount); - assertEq(erc20Rescuable.balanceOf(address(testedContract)), amount); - - vm.prank(owner); - vm.expectEmit(address(testedContract)); - emit BaseManaged.TokensRescued(address(erc20Rescuable), amount); - testedContract.rescueTokens(address(erc20Rescuable), amount, false); - - assertEq(erc20Rescuable.balanceOf(owner), amount); - assertEq(erc20Rescuable.balanceOf(address(testedContract)), 0); - } - - function test_rescueTokens_ERC721() public { - uint id = 1000; - erc721Rescuable.mint(address(testedContract), id); - assertEq(erc721Rescuable.ownerOf(id), address(testedContract)); - - vm.prank(owner); - vm.expectEmit(address(testedContract)); - emit BaseManaged.TokensRescued(address(erc721Rescuable), id); - testedContract.rescueTokens(address(erc721Rescuable), id, true); - - assertEq(erc721Rescuable.ownerOf(id), owner); - } - // reverts function test_revert_setConfigHub_invalidConfigHub() public { @@ -130,19 +86,6 @@ abstract contract BaseManagedTestBase is Test { vm.expectRevert("BaseManaged: not configHub owner"); testedContract.setConfigHub(configHub); - - vm.expectRevert("BaseManaged: not configHub owner"); - testedContract.rescueTokens(address(0), 0, true); - } - - function test_unrescuable() public { - vm.startPrank(owner); - vm.expectRevert("unrescuable asset"); - testedContract.rescueTokens(unrescuable, 1, false); - vm.expectRevert("unrescuable asset"); - testedContract.rescueTokens(unrescuable, 1, true); - vm.expectRevert("unrescuable asset"); - testedContract.rescueTokens(unrescuable, 0, true); } } @@ -162,65 +105,55 @@ contract BadConfigHub2 { // mock of an inheriting contract (because base is abstract) contract TestableBaseManaged is BaseManaged { - constructor(ConfigHub _configHub, address _unrescuable) BaseManaged(_configHub, _unrescuable) { } + constructor(ConfigHub _configHub) BaseManaged(_configHub) { } } // the tests for the mock contract contract BaseManagedMockTest is BaseManagedTestBase { function setupTestedContract() internal override { - testedContract = new TestableBaseManaged(configHub, unrescuable); + testedContract = new TestableBaseManaged(configHub); } function test_revert_constructor_invalidConfigHub() public { vm.expectRevert(new bytes(0)); - new TestableBaseManaged(ConfigHub(address(0)), unrescuable); + new TestableBaseManaged(ConfigHub(address(0))); ConfigHub badHub = ConfigHub(address(new BadConfigHub1())); vm.expectRevert(); - new TestableBaseManaged(badHub, unrescuable); + new TestableBaseManaged(badHub); badHub = ConfigHub(address(new BadConfigHub2(owner))); vm.expectRevert("invalid ConfigHub"); - new TestableBaseManaged(badHub, unrescuable); + new TestableBaseManaged(badHub); } } contract ProviderNFTManagedTest is BaseManagedTestBase { function setupTestedContract() internal override { - TestERC20 unrescuableErc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); - unrescuable = address(unrescuableErc20); - testedContract = new CollarProviderNFT( - configHub, unrescuableErc20, erc20Rescuable, address(0), "ProviderNFT", "ProviderNFT" - ); + TestERC20 erc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); + testedContract = + new CollarProviderNFT(configHub, erc20, erc20, address(0), "ProviderNFT", "ProviderNFT"); } } contract EscrowSupplierNFTManagedTest is BaseManagedTestBase { function setupTestedContract() internal override { - TestERC20 unrescuableErc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); - unrescuable = address(unrescuableErc20); - testedContract = new EscrowSupplierNFT(configHub, unrescuableErc20, "ProviderNFT", "ProviderNFT"); + TestERC20 erc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); + testedContract = new EscrowSupplierNFT(configHub, erc20, "ProviderNFT", "ProviderNFT"); } } contract TakerNFTManagedTest is BaseManagedTestBase { function setupTestedContract() internal virtual override { - TestERC20 unrescuableErc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); - unrescuable = address(unrescuableErc20); + TestERC20 erc20 = new TestERC20("TestERC20_2", "TestERC20_2", 18); + TestERC20 erc20_2 = new TestERC20("TestERC20_2", "TestERC20_2", 18); MockChainlinkFeed mockCLFeed = new MockChainlinkFeed(18, "TestFeed"); ChainlinkOracle oracle = new ChainlinkOracle( - address(erc20Rescuable), - address(unrescuableErc20), - address(mockCLFeed), - "TestFeed", - 60, - address(0) + address(erc20), address(erc20_2), address(mockCLFeed), "TestFeed", 60, address(0) ); // taker checks price on construction mockCLFeed.setLatestAnswer(1, 0); - testedContract = new CollarTakerNFT( - configHub, unrescuableErc20, erc20Rescuable, oracle, "CollarTakerNFT", "BRWTST" - ); + testedContract = new CollarTakerNFT(configHub, erc20_2, erc20, oracle, "CollarTakerNFT", "BRWTST"); } } @@ -229,7 +162,6 @@ contract LoansManagedTest is TakerNFTManagedTest { super.setupTestedContract(); // take the taker contract setup by the super CollarTakerNFT takerNFT = CollarTakerNFT(address(testedContract)); - unrescuable = address(takerNFT); testedContract = new LoansNFT(takerNFT, "", ""); } } diff --git a/test/unit/CollarProviderNFT.t.sol b/test/unit/CollarProviderNFT.t.sol index 9404d6a0..a31e6b5c 100644 --- a/test/unit/CollarProviderNFT.t.sol +++ b/test/unit/CollarProviderNFT.t.sol @@ -134,7 +134,6 @@ contract CollarProviderNFTTest is BaseAssetPairTestSetup { assertEq(address(newProviderNFT.configHubOwner()), owner); assertEq(address(newProviderNFT.configHub()), address(configHub)); - assertEq(newProviderNFT.unrescuableAsset(), address(cashAsset)); assertEq(address(newProviderNFT.cashAsset()), address(cashAsset)); assertEq(address(newProviderNFT.underlying()), address(underlying)); assertEq(address(newProviderNFT.taker()), takerContract); diff --git a/test/unit/CollarTakerNFT.t.sol b/test/unit/CollarTakerNFT.t.sol index b93840ec..3c827488 100644 --- a/test/unit/CollarTakerNFT.t.sol +++ b/test/unit/CollarTakerNFT.t.sol @@ -266,7 +266,6 @@ contract CollarTakerNFTTest is BaseAssetPairTestSetup { assertEq(takerNFT.configHubOwner(), owner); assertEq(address(takerNFT.configHub()), address(configHub)); - assertEq(takerNFT.unrescuableAsset(), address(cashAsset)); assertEq(address(takerNFT.cashAsset()), address(cashAsset)); assertEq(address(takerNFT.underlying()), address(underlying)); assertEq(address(takerNFT.oracle()), address(chainlinkOracle)); diff --git a/test/unit/EscrowSupplierNFT.effects.t.sol b/test/unit/EscrowSupplierNFT.effects.t.sol index afdb539a..caadb28c 100644 --- a/test/unit/EscrowSupplierNFT.effects.t.sol +++ b/test/unit/EscrowSupplierNFT.effects.t.sol @@ -288,7 +288,6 @@ contract EscrowSupplierNFT_BasicEffectsTest is BaseEscrowSupplierNFTTest { assertEq(address(newEscrowSupplierNFT.configHubOwner()), owner); assertEq(address(newEscrowSupplierNFT.configHub()), address(configHub)); - assertEq(newEscrowSupplierNFT.unrescuableAsset(), address(asset)); assertEq(address(newEscrowSupplierNFT.asset()), address(asset)); assertEq(newEscrowSupplierNFT.MAX_INTEREST_APR_BIPS(), BIPS_100PCT); assertEq(newEscrowSupplierNFT.MIN_GRACE_PERIOD(), 1 days); diff --git a/test/unit/Loans.basic.effects.t.sol b/test/unit/Loans.basic.effects.t.sol index 3310abb4..43bc55e0 100644 --- a/test/unit/Loans.basic.effects.t.sol +++ b/test/unit/Loans.basic.effects.t.sol @@ -441,7 +441,6 @@ contract LoansBasicEffectsTest is LoansTestBase { loans = new LoansNFT(takerNFT, "", ""); assertEq(loans.MAX_SWAP_PRICE_DEVIATION_BIPS(), 1000); assertEq(address(loans.configHub()), address(configHub)); - assertEq(loans.unrescuableAsset(), address(takerNFT)); assertEq(address(loans.takerNFT()), address(takerNFT)); assertEq(address(loans.cashAsset()), address(cashAsset)); assertEq(address(loans.underlying()), address(underlying)); From b6bb876129bf750b51b43e21d015117094149ef0 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Fri, 4 Apr 2025 17:29:17 +1100 Subject: [PATCH 07/12] Remove admin setKeeper method (#143) * remove admin setKeeper method * minor * comment fixes --- CHANGELOG.md | 2 + devdocs/audits.briefing.md | 2 +- src/CollarProviderNFT.sol | 4 +- src/CollarTakerNFT.sol | 6 +-- src/EscrowSupplierNFT.sol | 6 +-- src/LoansNFT.sol | 42 +++++++------------ src/interfaces/ILoansNFT.sol | 3 +- .../full-protocol/BaseAssetPairForkTest.sol | 1 - test/unit/Loans.admin.t.sol | 14 ------- test/unit/Loans.basic.effects.t.sol | 29 ++++++------- test/unit/Loans.basic.reverts.t.sol | 10 ++--- test/unit/Loans.rolls.reverts.t.sol | 4 +- 12 files changed, 45 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0b93f54..7a109cfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ CollarTakerNFT, LoansNFT, EscrowSupplierNFT, CollarProviderNFT: - Inheriting from updated BaseManaged (not ownable, not pausable). - Different constructor interface. - Using `onlyConfigHubOwner` for admin methods. +LoansNFT: + - removed the admin method for setting keeper, users can choose a keeper when approving CollarTakerNFT: - `setOracle` is removed, and `settleAsCancelled` (for oracle failure) is added CollarProviderNFT: diff --git a/devdocs/audits.briefing.md b/devdocs/audits.briefing.md index 5a8e275d..5e6d77b6 100644 --- a/devdocs/audits.briefing.md +++ b/devdocs/audits.briefing.md @@ -63,7 +63,7 @@ Base, possibly Eth L1 later. ## ConfigHub's owner privileges (for BaseManaged contracts) - Can update the values set in ConfigHub and replace the ConfigHub contract that's being used. This includes what internal contracts are allowed to open positions, LTV range, durations range, protocol fee parameters. -- Can update the loansNFT closing keeper address and allowed swappers. +- Can update the loansNFT allowed swappers. ## Testing and POC - Install run tests excluding fork tests: `forge install && forge build && forge test --nmc Fork` diff --git a/src/CollarProviderNFT.sol b/src/CollarProviderNFT.sol index 582a482e..2567d50a 100644 --- a/src/CollarProviderNFT.sol +++ b/src/CollarProviderNFT.sol @@ -35,8 +35,8 @@ import { ICollarProviderNFT } from "./interfaces/ICollarProviderNFT.sol"; * * Post-Deployment Configuration: * - ConfigHub: Properly configured LTV, duration, and protocol fee parameters - * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset pair - * - ConfigHub: Set setCanOpenPair() to authorize its paired taker contract + * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset pair [underlying, cash, provider] + * - ConfigHub: Set setCanOpenPair() to authorize its paired taker contract [underlying, cash, taker] */ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { using SafeERC20 for IERC20; diff --git a/src/CollarTakerNFT.sol b/src/CollarTakerNFT.sol index 74cdefd0..e1d21371 100644 --- a/src/CollarTakerNFT.sol +++ b/src/CollarTakerNFT.sol @@ -36,8 +36,8 @@ import { ICollarProviderNFT } from "./interfaces/ICollarProviderNFT.sol"; * * Post-Deployment Configuration: * - Oracle: If using Uniswap ensure adequate observation cardinality, if using Chainlink ensure correct config. - * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset pair - * - ConfigHub: Set setCanOpenPair() to authorize the provider contract + * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset pair [underlying, cash, taker] + * - ConfigHub: Set setCanOpenPair() to authorize the provider contract [underlying, cash, provider] * - CollarProviderNFT: Ensure properly configured */ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { @@ -302,7 +302,7 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { uint expectedBalance = cashAsset.balanceOf(address(this)); // call provider side to settle with no balance change providerNFT.settlePosition(providerId, 0); - // check balance update to prevent reducing resting balance + // check no balance change require(cashAsset.balanceOf(address(this)) == expectedBalance, "taker: settle balance mismatch"); // endPrice is 0 since is unavailable, technically settlement price is position.startPrice but diff --git a/src/EscrowSupplierNFT.sol b/src/EscrowSupplierNFT.sol index e0e1914b..379de266 100644 --- a/src/EscrowSupplierNFT.sol +++ b/src/EscrowSupplierNFT.sol @@ -35,9 +35,9 @@ import { IEscrowSupplierNFT } from "./interfaces/IEscrowSupplierNFT.sol"; * * Post-Deployment Configuration: * - ConfigHub: Set valid collar duration range - * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset - * - ConfigHub: Set setCanOpenPair() to authorize its loans contracts - * - This Contract: Set loans that can open escrows + * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset [underlying, ANY, escrow] + * - ConfigHub: Set setCanOpenPair() to authorize loans contracts to open positions [underlying, cash, loans] + * - ConfigHub: Set setCanOpenPair() to authorize loans contract to open escrow here [underlying, escrow, loans] */ contract EscrowSupplierNFT is IEscrowSupplierNFT, BaseNFT { using SafeERC20 for IERC20; diff --git a/src/LoansNFT.sol b/src/LoansNFT.sol index 2cba9b6c..fc5edae4 100644 --- a/src/LoansNFT.sol +++ b/src/LoansNFT.sol @@ -34,13 +34,13 @@ import { ILoansNFT } from "./interfaces/ILoansNFT.sol"; * according to transfer arguments (no rebasing, no FoT), 0 value approvals and transfers work. * * Post-Deployment Configuration: - * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset pair - * - ConfigHub: Set setCanOpenPair() to authorize: taker, provider, rolls contracts for the asset pair. - * - ConfigHub: If allowing escrow, set setCanOpenPair() to authorize escrow for underlying and ANY_ASSET. + * - ConfigHub: Set setCanOpenPair() to authorize this contract for its asset pair [underlying, cash, loans] + * - ConfigHub: Set setCanOpenPair() to authorize: taker, provider, rolls for pair [underlying, cash, ...] + * - ConfigHub: If allowing escrow, set setCanOpenPair() to authorize escrow [underlying, ANY_ASSET, escrow]. + * - ConfigHub: If allowing escrow, set setCanOpenPair() to authorize loans for escrow [underlying, escrow, loans]. * - CollarTakerNFT and CollarProviderNFT: Ensure properly configured * - EscrowSupplierNFT: If allowing escrow, ensure properly configured * - This Contract: Set an allowed default swapper - * - This Contract: Set allowed closing keeper if using keeper functionality */ contract LoansNFT is ILoansNFT, BaseNFT { using SafeERC20 for IERC20; @@ -62,12 +62,9 @@ contract LoansNFT is ILoansNFT, BaseNFT { /// @notice Stores loan information for each NFT ID mapping(uint loanId => LoanStored) internal loans; // borrowers that allow a keeper for loan closing for specific loans - mapping(address sender => mapping(uint loanId => bool enabled)) public keeperApprovedFor; + mapping(address sender => mapping(uint loanId => address keeper)) public keeperApprovedFor; // ----- Admin state ----- // - // optional keeper (set by contract owner) that's useful for the time-sensitive - // swap back during loan closing - address public closingKeeper; // contracts allowed for swaps EnumerableSet.AddressSet internal allowedSwappers; // a convenience view to allow querying for a swapper onchain / FE without subgraph @@ -352,29 +349,24 @@ contract LoansNFT is ILoansNFT, BaseNFT { } /** - * @notice Allows or disallows a keeper for closing a specific loan on behalf of a caller + * @notice Sets a keeper for closing a specific loan on behalf of a caller. * A user that sets this allowance with the intention for the keeper to closeLoan * has to also ensure cash approval to this contract that should be valid when * closeLoan is called by the keeper. + * To unset, set keeper to address(0). + * If the loan is transferred to another owner, this approval will become invalid, because the loan + * is held by someone else. However, if it is transferred back to the original approver, the approval + * will become valid again. * @param loanId specific loanId which the user approves the keeper to close - * @param enabled True to allow the keeper, false to disallow + * @param keeper address of the keeper the user approves */ - function setKeeperApproved(uint loanId, bool enabled) external { - keeperApprovedFor[msg.sender][loanId] = enabled; - emit ClosingKeeperApproved(msg.sender, loanId, enabled); + function setKeeperFor(uint loanId, address keeper) external { + keeperApprovedFor[msg.sender][loanId] = keeper; + emit ClosingKeeperApproved(msg.sender, loanId, keeper); } // ----- Admin methods ----- // - /// @notice Sets the address of the single allowed closing keeper. Alternative decentralized keeper - /// arrangements can be added via composability, e.g., a contract that would hold the NFTs (thus - /// will be able to perform actions) and allow incentivised keepers to call it. - /// @dev only configHub owner - function setKeeper(address keeper) external onlyConfigHubOwner { - emit ClosingKeeperUpdated(closingKeeper, keeper); - closingKeeper = keeper; - } - /// @notice Enables or disables swappers and sets the defaultSwapper view. /// When no swapper is allowed, opening and closing loans will not be possible, only cancelling. /// The default swapper is a convenience view, and it's best to keep it up to date @@ -782,11 +774,7 @@ contract LoansNFT is ILoansNFT, BaseNFT { their loans that are about to be closed, and only via a more complex "slippage" attack. */ function _isSenderOrKeeperFor(address authorizedSender, uint loanId) internal view returns (bool) { - bool isSender = msg.sender == authorizedSender; // is the auth target - bool isKeeper = msg.sender == closingKeeper; - // our auth target allows the keeper - bool _keeperApproved = keeperApprovedFor[authorizedSender][loanId]; - return isSender || (_keeperApproved && isKeeper); + return msg.sender == authorizedSender || msg.sender == keeperApprovedFor[authorizedSender][loanId]; } function _newLoanIdCheck(uint takerId) internal view returns (uint loanId) { diff --git a/src/interfaces/ILoansNFT.sol b/src/interfaces/ILoansNFT.sol index a1bedc94..5b81c8f1 100644 --- a/src/interfaces/ILoansNFT.sol +++ b/src/interfaces/ILoansNFT.sol @@ -79,8 +79,7 @@ interface ILoansNFT { uint escrowId ); event LoanCancelled(uint indexed loanId, address indexed sender); - event ClosingKeeperApproved(address indexed sender, uint indexed loanId, bool indexed enabled); - event ClosingKeeperUpdated(address indexed previousKeeper, address indexed newKeeper); + event ClosingKeeperApproved(address indexed sender, uint indexed loanId, address indexed keeper); event SwapperSet(address indexed swapper, bool indexed allowed, bool indexed setDefault); event EscrowSettled(uint indexed escrowId, uint toEscrow, uint fromEscrow, uint leftOver); } diff --git a/test/integration/full-protocol/BaseAssetPairForkTest.sol b/test/integration/full-protocol/BaseAssetPairForkTest.sol index 1d955396..bd75045a 100644 --- a/test/integration/full-protocol/BaseAssetPairForkTest.sol +++ b/test/integration/full-protocol/BaseAssetPairForkTest.sol @@ -394,7 +394,6 @@ abstract contract BaseAssetPairForkTest is Test { address[] memory oneSwapper = new address[](1); oneSwapper[0] = address(pair.swapperUniV3); assertEq(pair.loansContract.allAllowedSwappers(), oneSwapper); - assertEq(pair.loansContract.closingKeeper(), address(0)); // escrow assertEq(pair.escrowNFT.VERSION(), "0.3.0"); diff --git a/test/unit/Loans.admin.t.sol b/test/unit/Loans.admin.t.sol index 61ea06f4..71f43555 100644 --- a/test/unit/Loans.admin.t.sol +++ b/test/unit/Loans.admin.t.sol @@ -20,24 +20,10 @@ contract LoansAdminTest is LoansTestBase { function test_onlyConfigHubOwnerMethods() public { vm.startPrank(user1); - vm.expectRevert("BaseManaged: not configHub owner"); - loans.setKeeper(keeper); - vm.expectRevert("BaseManaged: not configHub owner"); loans.setSwapperAllowed(address(0), true, true); } - function test_setKeeper() public { - assertEq(loans.closingKeeper(), address(0)); - - vm.startPrank(owner); - vm.expectEmit(address(loans)); - emit ILoansNFT.ClosingKeeperUpdated(address(0), keeper); - loans.setKeeper(keeper); - - assertEq(loans.closingKeeper(), keeper); - } - function test_setSwapperAllowed() public { vm.startPrank(owner); diff --git a/test/unit/Loans.basic.effects.t.sol b/test/unit/Loans.basic.effects.t.sol index 43bc55e0..bf7729a1 100644 --- a/test/unit/Loans.basic.effects.t.sol +++ b/test/unit/Loans.basic.effects.t.sol @@ -446,7 +446,6 @@ contract LoansBasicEffectsTest is LoansTestBase { assertEq(address(loans.underlying()), address(underlying)); assertEq(loans.VERSION(), "0.3.0"); assertEq(loans.configHubOwner(), owner); - assertEq(loans.closingKeeper(), address(0)); assertEq(address(loans.defaultSwapper()), address(0)); assertEq(loans.name(), ""); assertEq(loans.symbol(), ""); @@ -495,24 +494,24 @@ contract LoansBasicEffectsTest is LoansTestBase { assertEq(loans.tokenURI(loanId), expected); } - function test_allowsClosingKeeper() public { + function test_setKeeperFor() public { uint loanId = 100; uint otherLoanId = 50; startHoax(user1); - assertFalse(loans.keeperApprovedFor(user1, loanId)); - assertFalse(loans.keeperApprovedFor(user1, otherLoanId)); + assertEq(loans.keeperApprovedFor(user1, loanId), address(0)); + assertEq(loans.keeperApprovedFor(user1, otherLoanId), address(0)); vm.expectEmit(address(loans)); - emit ILoansNFT.ClosingKeeperApproved(user1, loanId, true); - loans.setKeeperApproved(loanId, true); - assertTrue(loans.keeperApprovedFor(user1, loanId)); - assertFalse(loans.keeperApprovedFor(user1, otherLoanId)); + emit ILoansNFT.ClosingKeeperApproved(user1, loanId, keeper); + loans.setKeeperFor(loanId, keeper); + assertEq(loans.keeperApprovedFor(user1, loanId), keeper); + assertEq(loans.keeperApprovedFor(user1, otherLoanId), address(0)); vm.expectEmit(address(loans)); - emit ILoansNFT.ClosingKeeperApproved(user1, loanId, false); - loans.setKeeperApproved(loanId, false); - assertFalse(loans.keeperApprovedFor(user1, loanId)); - assertFalse(loans.keeperApprovedFor(user1, otherLoanId)); + emit ILoansNFT.ClosingKeeperApproved(user1, loanId, address(0)); + loans.setKeeperFor(loanId, address(0)); + assertEq(loans.keeperApprovedFor(user1, loanId), address(0)); + assertEq(loans.keeperApprovedFor(user1, otherLoanId), address(0)); } function test_closeLoan_simple() public { @@ -598,13 +597,9 @@ contract LoansBasicEffectsTest is LoansTestBase { (uint loanId,, uint loanAmount) = createAndCheckLoan(); skip(duration); - // Set the keeper - vm.startPrank(owner); - loans.setKeeper(keeper); - // Allow the keeper to close the loan vm.startPrank(user1); - loans.setKeeperApproved(loanId, true); + loans.setKeeperFor(loanId, keeper); CollarTakerNFT.TakerPosition memory takerPosition = takerNFT.getPosition({ takerId: loanId }); // withdrawal: no price change so only user locked (put locked) diff --git a/test/unit/Loans.basic.reverts.t.sol b/test/unit/Loans.basic.reverts.t.sol index a6aa6c3b..96981f82 100644 --- a/test/unit/Loans.basic.reverts.t.sol +++ b/test/unit/Loans.basic.reverts.t.sol @@ -292,16 +292,13 @@ contract LoansBasicRevertsTest is LoansTestBase { (uint loanId,, uint loanAmount) = createAndCheckLoan(); skip(duration); - vm.startPrank(owner); - loans.setKeeper(keeper); - vm.startPrank(keeper); // keeper was not allowed by user vm.expectRevert("loans: not NFT owner or allowed keeper"); loans.closeLoan(loanId, defaultSwapParams(0)); vm.startPrank(user1); - loans.setKeeperApproved(0, true); + loans.setKeeperFor(0, keeper); // approved wrong loan vm.startPrank(keeper); vm.expectRevert("loans: not NFT owner or allowed keeper"); @@ -309,7 +306,7 @@ contract LoansBasicRevertsTest is LoansTestBase { // correct loan vm.startPrank(user1); - loans.setKeeperApproved(loanId, true); + loans.setKeeperFor(loanId, keeper); vm.startPrank(user1); // transfer invalidates approval @@ -322,6 +319,9 @@ contract LoansBasicRevertsTest is LoansTestBase { // transfer back vm.startPrank(provider); loans.transferFrom(provider, user1, loanId); + // not keeper cannot call (provider is calling) + vm.expectRevert("loans: not NFT owner or allowed keeper"); + loans.closeLoan(loanId, defaultSwapParams(0)); // should work now vm.startPrank(user1); diff --git a/test/unit/Loans.rolls.reverts.t.sol b/test/unit/Loans.rolls.reverts.t.sol index dfe17f78..e3e4820b 100644 --- a/test/unit/Loans.rolls.reverts.t.sol +++ b/test/unit/Loans.rolls.reverts.t.sol @@ -174,12 +174,10 @@ contract LoansRollsRevertsTest is LoansRollTestBase { // Create a loan (uint loanId,,) = createAndCheckLoan(); uint rollId = createRollOffer(loanId); - vm.startPrank(owner); - loans.setKeeper(keeper); // Allow the keeper to close the loan (but not roll) vm.startPrank(user1); - loans.setKeeperApproved(loanId, true); + loans.setKeeperFor(loanId, keeper); // Attempt to roll the loan as the keeper vm.startPrank(keeper); From 3baeb575f42affa5a178c8c03448abc39c15bc25 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Wed, 9 Apr 2025 10:04:47 +1000 Subject: [PATCH 08/12] BaseSep deployment for 0.3.0 (#144) * deployment artifacts * enable NoDeploy fork tests for base-sep * add minor deploy validation check * minor script update * fix the new check if shared escrow --- ...lia_collar_protocol_deployment-latest.json | 30 +++++++++---------- script/utils/explorer-links.sh | 3 ++ .../full-protocol/BaseAssetPairForkTest.sol | 28 +++++++++++++++++ .../full-protocol/Loans.fork.opbase.t.sol | 4 --- 4 files changed, 46 insertions(+), 19 deletions(-) diff --git a/script/artifacts/84532/opbase_sepolia_collar_protocol_deployment-latest.json b/script/artifacts/84532/opbase_sepolia_collar_protocol_deployment-latest.json index 99be0052..a69d08a2 100644 --- a/script/artifacts/84532/opbase_sepolia_collar_protocol_deployment-latest.json +++ b/script/artifacts/84532/opbase_sepolia_collar_protocol_deployment-latest.json @@ -1,22 +1,22 @@ { - "configHub": "0x30CaAe57aB5A0D5778277cB60a349344B710E254", - "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_takerNFT": "0x6A6d7521c783D71855c0867D7710E3D95E677393", - "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_providerNFT": "0xE58e48135fD1A8e16c141BD5C8Ce84537c2db135", - "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_loansContract": "0x3Fa73aD9eC3527f1444283476CA74B81Cbb8e5b9", - "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_escrowSupplierNFT": "0xc76a1bc79B876F8068fa695600c7A1A3E2f6545b", - "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_rollsContract": "0x76850e70542CAbC29eeCCe74DE6d88340Ce4F55A", - "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_oracle": "0xe86578D8feF3c70cB4FaccbD2DEbC66EA1d4B373", - "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_swapperUniV3": "0x25911776584d581F5457e77eF868F6d1B9371882", + "configHub": "0x6ce02fBbEa5E933aea9565A630678A5F68432109", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_takerNFT": "0x36d0FACE4E76C39a104A03c39738a69b946F6EAD", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_providerNFT": "0x7E4a569578C98C120b580D84bf94949aE598C2F1", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_loansContract": "0x2eaabba4714E7EC1FCbBaD77566E91F9ab093eAD", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_escrowSupplierNFT": "0xC89cFC1fe84A5f5260B5F753E070296dF5B07049", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_rollsContract": "0xeb7C796D2F703811da5150bCF241D7F4fF9aa08c", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_oracle": "0x534C7535454250574161c4ab0E23356a939E9530", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_swapperUniV3": "0x1cB3aAf674578C3f486e6Fed6Cf9E89c4D3e7A71", "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_cashAsset": "0x17F5E1f30871D487612331d674765F610324a532", "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_underlying": "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec", "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_swapFeeTier": 500, - "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_takerNFT": "0x8f66ABE3012EB2B41429735723AFe8f577165f2E", - "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_providerNFT": "0xA210d1B753f4c27567616Fc2526f7d049e71F981", - "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_loansContract": "0x1807Fc1080D88d3a29aa01E9B975eB7967120d6d", - "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_escrowSupplierNFT": "0x924E1c13B28f5C083Fa7a7972a14fF3A62011985", - "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_rollsContract": "0xD821D17b2A32f651c58465EcDda97151bFCD1CF5", - "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_oracle": "0x7e7a725FC89074E57b17977Cf5FAA867e619c64f", - "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_swapperUniV3": "0x8b41cAdf6A85ff93De9946531d7C8867D6F83fB3", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_takerNFT": "0x759601AEc5bE0621E15BFBE47899e466f4e8f27d", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_providerNFT": "0xA220fC553B8Ae45F0c4891be53326A91D66497dB", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_loansContract": "0xAAF7A36731ea5A9E58066e24d909dD32a73d4D22", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_escrowSupplierNFT": "0xEb4fD19D21afBCB5DF538D264cBC83993Cc158CB", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_rollsContract": "0xd4909d4dE2E5Db0fD8716CD2913556224649b7DB", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_oracle": "0xCA1BBACe3aC0c68b7AFd174B5C8B85E3490373De", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_swapperUniV3": "0x54968f818cDab93de4898A6322cB6d3C987023e2", "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_cashAsset": "0x17F5E1f30871D487612331d674765F610324a532", "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_underlying": "0x25361aD7C93F46e71434940d705815bD38BB0fa3", "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_swapFeeTier": 500 diff --git a/script/utils/explorer-links.sh b/script/utils/explorer-links.sh index 55b785db..a612788a 100644 --- a/script/utils/explorer-links.sh +++ b/script/utils/explorer-links.sh @@ -7,6 +7,9 @@ BROADCAST_FILE="broadcast/deploy-protocol.s.sol/8453/dry-run/run-latest.json" EXPLORER_BASE_URL="https://basescan.org/address/" +#BROADCAST_FILE="broadcast/deploy-protocol.s.sol/84532/dry-run/run-latest.json" +#EXPLORER_BASE_URL="https://sepolia.basescan.org/address/" + echo "# Deployed contracts" ## simple lines diff --git a/test/integration/full-protocol/BaseAssetPairForkTest.sol b/test/integration/full-protocol/BaseAssetPairForkTest.sol index bd75045a..c4969a0e 100644 --- a/test/integration/full-protocol/BaseAssetPairForkTest.sol +++ b/test/integration/full-protocol/BaseAssetPairForkTest.sol @@ -346,6 +346,27 @@ abstract contract BaseAssetPairForkTest is Test { assertEq(pair.underlying.balanceOf(user) - userUnderlyingBefore, underlyingOut); } + // addresses of all loans contracts expected to be using the escrow for a particular underlying + function expectedApprovedLoansForEscrow(IERC20 _underlying) + internal + view + returns (address[] memory expectedLoans) + { + // define a array that's too long, and truncate after the loop (otherwise need to do two ugly loops) + expectedLoans = new address[](deployedPairs.length); + uint nPairsForUnderlying; + for (uint i = 0; i < deployedPairs.length; i++) { + if (deployedPairs[i].underlying == _underlying) { + expectedLoans[nPairsForUnderlying] = address(deployedPairs[i].loansContract); + nPairsForUnderlying++; + } + } + // truncate the memory array to the right length + assembly { + mstore(expectedLoans, nPairsForUnderlying) + } + } + // tests function test_validatePairDeployment() public view { @@ -427,6 +448,8 @@ abstract contract BaseAssetPairForkTest is Test { ); // all pair auth + + // underlying x cash -> pair address[] memory pairAuthed = new address[](4); pairAuthed[0] = address(pair.takerNFT); pairAuthed[1] = address(pair.providerNFT); @@ -434,6 +457,11 @@ abstract contract BaseAssetPairForkTest is Test { pairAuthed[3] = address(pair.rollsContract); assertEq(configHub.allCanOpenPair(address(pair.underlying), address(pair.cashAsset)), pairAuthed); + // underlying x escrow -> loans + // multiple loans contracts can be using the same escrow + address[] memory loansToEscrow = expectedApprovedLoansForEscrow(pair.underlying); + assertEq(configHub.allCanOpenPair(address(pair.underlying), address(pair.escrowNFT)), loansToEscrow); + // single asset auth assertTrue(configHub.canOpenSingle(address(pair.underlying), address(pair.escrowNFT))); diff --git a/test/integration/full-protocol/Loans.fork.opbase.t.sol b/test/integration/full-protocol/Loans.fork.opbase.t.sol index 0f107cc3..6d38bbfc 100644 --- a/test/integration/full-protocol/Loans.fork.opbase.t.sol +++ b/test/integration/full-protocol/Loans.fork.opbase.t.sol @@ -206,8 +206,6 @@ contract TWETHTUSDC_OPBaseSep_LoansForkTest_NoDeploy is TWETHTUSDC_OPBaseSep_Loa super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; - // TODO: enable test when new release is deployed - vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseSep_artifactsName; } } @@ -233,8 +231,6 @@ contract TWBTCTUSDC_OPBaseSep_LoansForkTest_NoDeploy is TWBTCTUSDC_OPBaseSep_Loa super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; - // TODO: enable test when new release is deployed - vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseSep_artifactsName; } } From 71f47eef0e65eb1697b8c7354478802fbe75f362 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Thu, 10 Apr 2025 09:53:44 +1000 Subject: [PATCH 09/12] fix fork tests :( (#146) --- test/integration/full-protocol/Loans.fork.arbi.t.sol | 4 ++-- test/integration/full-protocol/Loans.fork.opbase.t.sol | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/integration/full-protocol/Loans.fork.arbi.t.sol b/test/integration/full-protocol/Loans.fork.arbi.t.sol index e1d12e62..1bd3ec3e 100644 --- a/test/integration/full-protocol/Loans.fork.arbi.t.sol +++ b/test/integration/full-protocol/Loans.fork.arbi.t.sol @@ -54,7 +54,7 @@ contract WETHUSDC_ArbiMain_LoansForkTest is BaseAssetPairForkTest_ScriptTest { slippage = 100; // 1% callstrikeToUse = 11_000; - expectedOraclePrice = 3_000_000_000; + expectedOraclePrice = 2_000_000_000; } } @@ -119,7 +119,7 @@ contract WETHUSDC_ArbiSep_LoansForkTest is WETHUSDC_ArbiMain_LoansForkTest { cashAsset = Const.ArbiSep_tUSDC; oracleDescription = "Comb(CL(TWAPMock(ETH / USD))|inv(CL(FixedMock(USDC / USD))))"; - expectedOraclePrice = 3_000_000_000; + expectedOraclePrice = 2_000_000_000; } function deployFullProtocol() diff --git a/test/integration/full-protocol/Loans.fork.opbase.t.sol b/test/integration/full-protocol/Loans.fork.opbase.t.sol index 6d38bbfc..a932a6fb 100644 --- a/test/integration/full-protocol/Loans.fork.opbase.t.sol +++ b/test/integration/full-protocol/Loans.fork.opbase.t.sol @@ -110,7 +110,7 @@ contract WETHUSDC_OPBaseMain_LoansForkTest is BaseAssetPairForkTest_ScriptTest { slippage = 100; // 1% callstrikeToUse = 11_000; - expectedOraclePrice = 3_000_000_000; + expectedOraclePrice = 2_000_000_000; } } @@ -197,7 +197,7 @@ contract TWETHTUSDC_OPBaseSep_LoansForkTest is WETHUSDC_OPBaseMain_LoansForkTest cashAsset = Const.OPBaseSep_tUSDC; oracleDescription = "Comb(CL(TWAPMock(ETH / USD))|inv(CL(FixedMock(USDC / USD))))"; - expectedOraclePrice = 3_500_000_000; // 3.5k in 1e6 + expectedOraclePrice = 2_000_000_000; // 2k in 1e6 } } From b6b4f8fcb2136c7ef5f6de00ac912abac1d61dd8 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Sat, 12 Apr 2025 10:28:27 +1000 Subject: [PATCH 10/12] Script for close-only mode (deprecating a deployment) (#147) * fix fork tests :( * script for close-only mode for deprecated contracts --- ...lia_collar_protocol_deployment-latest.json | 23 +++++ script/deploy/set-close-only-mode.s.sol | 96 +++++++++++++++++++ script/libraries/BaseDeployer.sol | 35 +++++-- 3 files changed, 146 insertions(+), 8 deletions(-) create mode 100644 script/artifacts/84532/DEPRECATED-opbase_sepolia_collar_protocol_deployment-latest.json create mode 100644 script/deploy/set-close-only-mode.s.sol diff --git a/script/artifacts/84532/DEPRECATED-opbase_sepolia_collar_protocol_deployment-latest.json b/script/artifacts/84532/DEPRECATED-opbase_sepolia_collar_protocol_deployment-latest.json new file mode 100644 index 00000000..77d1cb7b --- /dev/null +++ b/script/artifacts/84532/DEPRECATED-opbase_sepolia_collar_protocol_deployment-latest.json @@ -0,0 +1,23 @@ +{ + "configHub": "0x30CaAe57aB5A0D5778277cB60a349344B710E254", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_takerNFT": "0x6A6d7521c783D71855c0867D7710E3D95E677393", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_providerNFT": "0xE58e48135fD1A8e16c141BD5C8Ce84537c2db135", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_loansContract": "0x3Fa73aD9eC3527f1444283476CA74B81Cbb8e5b9", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_escrowSupplierNFT": "0xc76a1bc79B876F8068fa695600c7A1A3E2f6545b", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_rollsContract": "0x76850e70542CAbC29eeCCe74DE6d88340Ce4F55A", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_oracle": "0xe86578D8feF3c70cB4FaccbD2DEbC66EA1d4B373", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_swapperUniV3": "0x25911776584d581F5457e77eF868F6d1B9371882", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_cashAsset": "0x17F5E1f30871D487612331d674765F610324a532", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_underlying": "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec", + "0xA703Bb2faf4A977E9867DcbfC4c141c0a50F3Aec_0x17F5E1f30871D487612331d674765F610324a532_swapFeeTier": 500, + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_takerNFT": "0x8f66ABE3012EB2B41429735723AFe8f577165f2E", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_providerNFT": "0xA210d1B753f4c27567616Fc2526f7d049e71F981", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_loansContract": "0x1807Fc1080D88d3a29aa01E9B975eB7967120d6d", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_escrowSupplierNFT": "0x924E1c13B28f5C083Fa7a7972a14fF3A62011985", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_rollsContract": "0xD821D17b2A32f651c58465EcDda97151bFCD1CF5", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_oracle": "0x7e7a725FC89074E57b17977Cf5FAA867e619c64f", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_swapperUniV3": "0x8b41cAdf6A85ff93De9946531d7C8867D6F83fB3", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_cashAsset": "0x17F5E1f30871D487612331d674765F610324a532", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_underlying": "0x25361aD7C93F46e71434940d705815bD38BB0fa3", + "0x25361aD7C93F46e71434940d705815bD38BB0fa3_0x17F5E1f30871D487612331d674765F610324a532_swapFeeTier": 500 +} diff --git a/script/deploy/set-close-only-mode.s.sol b/script/deploy/set-close-only-mode.s.sol new file mode 100644 index 00000000..eef480f7 --- /dev/null +++ b/script/deploy/set-close-only-mode.s.sol @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.22; + +import "forge-std/Script.sol"; +import "forge-std/console.sol"; +import { DeploymentArtifactsLib } from "../libraries/DeploymentArtifacts.sol"; +import { BaseDeployer, ConfigHub } from "../libraries/BaseDeployer.sol"; +import { Const } from "../utils/Const.sol"; + +/** + * This simulates set close-only mode (i.e. disabling opening positions) on an outdated deployment from owner for: + * - Creating a broadcast artifact that can be used to create the Safe batch. + * - Simulating the above step in fork tests + * + * To create the Safe batch using this: + * 1. Run the script as dry-run (i.e. without --broadcast) + * 2. Run jq transform on the script's output JSON ("Transactions saved to: .."): + * `jq -f script/utils/safe-batch-from-broadcast.jq | tee temp-safe-batch.json' + * 3. Load `temp-safe-batch.json` in Safe > Transaction Builder > choose a file + * 4. Verify, simulate, inspect tenderly, create, inspect, and submit for signers to verify. + */ +abstract contract SetCloseOnlyModeScript is Script { + address public owner; + string public artifactsName; + + function run() external { + setParams(); + + // this is hardcoded to the owner: + // - in script usage this simulates and saves the txs batch to be imported + // into the Safe + // - in test usage this works as a prank for simulating the safe executing it + vm.startBroadcast(owner); + + // load the deployment artifacts + BaseDeployer.DeploymentResult memory deployment; + (deployment.configHub, deployment.assetPairContracts) = + DeploymentArtifactsLib.loadHubAndAllPairs(vm, artifactsName); + + for (uint i = 0; i < deployment.assetPairContracts.length; i++) { + BaseDeployer.disableContractPair(deployment.configHub, deployment.assetPairContracts[i]); + } + + vm.stopBroadcast(); + + // @dev this only happens in simulation (when broadcast txs are being collected), so this validates + // vs the simulated state (fake, local) - while the actual onchain state is still different (would not pass). + // The actual state will be changed if and when the transactions are actually executed (and succeed). + validateCloseOnlyMode(deployment); + } + + function setParams() internal virtual; + + function validateCloseOnlyMode(BaseDeployer.DeploymentResult memory deployment) internal view { + // this can be done in fork tests too, but it makes sense to validate what's easy here + for (uint i = 0; i < deployment.assetPairContracts.length; i++) { + BaseDeployer.AssetPairContracts memory pair = deployment.assetPairContracts[i]; + + // we do NOT want to disable swappers, since closing should be possible + require(pair.loansContract.allAllowedSwappers().length != 0, "Loans allAllowedSwappers is empty"); + + // check all canOpenPair auth tuples for pair are empty + ConfigHub hub = deployment.configHub; + address[] memory arr = hub.allCanOpenPair(address(pair.underlying), address(pair.cashAsset)); + require(arr.length == 0, "ConfigHub's allCanOpenPair not empty for pair"); + + // checking the above for a pair should be sufficient to ensure no positions can be opened, + // but we can check more combinations here, so why not + arr = hub.allCanOpenPair(address(pair.cashAsset), address(pair.underlying)); + require(arr.length == 0, "ConfigHub's allCanOpenPair not empty for pair (reverse)"); + + arr = hub.allCanOpenPair(address(pair.underlying), deployment.configHub.ANY_ASSET()); + require(arr.length == 0, "ConfigHub's allCanOpenPair not empty for underlying & ANY"); + + arr = hub.allCanOpenPair(address(pair.cashAsset), deployment.configHub.ANY_ASSET()); + require(arr.length == 0, "ConfigHub's allCanOpenPair not empty for cashAsset & ANY"); + + arr = hub.allCanOpenPair(address(pair.underlying), address(pair.escrowNFT)); + require(arr.length == 0, "ConfigHub's allCanOpenPair not empty for underlying & escrow"); + } + } +} + +contract SetCloseOnlyModeOPBaseMainnet is SetCloseOnlyModeScript { + function setParams() internal override { + owner = Const.OPBaseMain_owner; + artifactsName = string.concat("DEPRECATED-", Const.OPBaseMain_artifactsName); + } +} + +contract SetCloseOnlyModeOPBaseSepolia is SetCloseOnlyModeScript { + function setParams() internal override { + owner = Const.OPBaseSep_owner; + artifactsName = string.concat("DEPRECATED-", Const.OPBaseSep_artifactsName); + } +} diff --git a/script/libraries/BaseDeployer.sol b/script/libraries/BaseDeployer.sol index 63d03bc1..77a9bdba 100644 --- a/script/libraries/BaseDeployer.sol +++ b/script/libraries/BaseDeployer.sol @@ -161,21 +161,40 @@ library BaseDeployer { } function setupContractPair(ConfigHub hub, AssetPairContracts memory pair) internal { - hub.setCanOpenPair(address(pair.underlying), address(pair.cashAsset), address(pair.takerNFT), true); - hub.setCanOpenPair(address(pair.underlying), address(pair.cashAsset), address(pair.providerNFT), true); + setContractsAuth(hub, pair, true); + + // setup initial and default swapper + pair.loansContract.setSwapperAllowed(address(pair.swapperUniV3), true, true); + } + + // @dev note that when disabling previous version, it's auth interfaces may be different + // from current (either in contracts or in ConfigHub), so more actions may be needed, or + // some actions may fail + function disableContractPair(ConfigHub hub, AssetPairContracts memory pair) internal { + setContractsAuth(hub, pair, false); + + // @dev swappers are not touched, since closing position should still be possible + } + + function setContractsAuth(ConfigHub hub, AssetPairContracts memory pair, bool enabled) internal { + // underlying x cash -> pair contracts + hub.setCanOpenPair(address(pair.underlying), address(pair.cashAsset), address(pair.takerNFT), enabled); hub.setCanOpenPair( - address(pair.underlying), address(pair.cashAsset), address(pair.loansContract), true + address(pair.underlying), address(pair.cashAsset), address(pair.providerNFT), enabled ); hub.setCanOpenPair( - address(pair.underlying), address(pair.cashAsset), address(pair.rollsContract), true + address(pair.underlying), address(pair.cashAsset), address(pair.loansContract), enabled + ); + hub.setCanOpenPair( + address(pair.underlying), address(pair.cashAsset), address(pair.rollsContract), enabled ); - hub.setCanOpenPair(address(pair.underlying), hub.ANY_ASSET(), address(pair.escrowNFT), true); - pair.loansContract.setSwapperAllowed(address(pair.swapperUniV3), true, true); + // underlying x any -> escrow + hub.setCanOpenPair(address(pair.underlying), hub.ANY_ASSET(), address(pair.escrowNFT), enabled); - // allow loans to open escrow positions + // underlying x escrow -> loans hub.setCanOpenPair( - address(pair.underlying), address(pair.escrowNFT), address(pair.loansContract), true + address(pair.underlying), address(pair.escrowNFT), address(pair.loansContract), enabled ); } From ca9ffd7ecd382f49b3c792d5ac992cdbc434b084 Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Wed, 16 Apr 2025 08:41:02 +1000 Subject: [PATCH 11/12] Audit fixes for 0.3.0 (#148) * remove pausing related comments * add provider position values comment * additional comment changes * minor code changes * add note re settle as cancelled and underlying --- devdocs/audits.briefing.md | 1 + src/CollarProviderNFT.sol | 4 ++-- src/CollarTakerNFT.sol | 4 +++- src/ConfigHub.sol | 12 +++++------- src/base/BaseManaged.sol | 2 +- 5 files changed, 12 insertions(+), 11 deletions(-) diff --git a/devdocs/audits.briefing.md b/devdocs/audits.briefing.md index 5e6d77b6..660df1a1 100644 --- a/devdocs/audits.briefing.md +++ b/devdocs/audits.briefing.md @@ -44,6 +44,7 @@ Base, possibly Eth L1 later. - Because oracle prices undergo multiple conversions (feeds, tokens units), asset and price feed combinations w.r.t to decimals and price ranges (e.g., low price tokens) are assumed to be checked to allow sufficient precision. - In case of congestion, calls for `openPairedPosition` (`openLoan` that uses it), and rolls `executeRoll` can be executed at higher price than the user intended (if price is lower, `openLoan` and `executeRoll` have slippage protection, and `openPairedPosition` has better upside for the caller). This is accepted as low likelihood, and low impact: loss is small since short congestion will result in small price change vs. original intent, and long downtime may fail the oracle sequencer uptime check. - If an oracle becomes malicious, there isn't a way to "unset" it. ConfigHub can prevent opening new positions for that pair, but existing positions will remain vulnerable. +- If a collar position is settled via `settleAsCancelled` (due an oracle malfunction, or no one calling regular settle for one week), the Loan using that position will still be possible to close, but the amount of underlying returned may not correspond well to current price (because the collar position will be settled at its opening price). The loan can also be cancelled if desired. - Any tokens accidentally sent to any of the contracts cannot be rescued. - Issues and considerations explained in the Solidity comments and audit reports. diff --git a/src/CollarProviderNFT.sol b/src/CollarProviderNFT.sol index 2567d50a..e90b9055 100644 --- a/src/CollarProviderNFT.sol +++ b/src/CollarProviderNFT.sol @@ -128,7 +128,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { /** * @notice Calculates the protocol fee charged from offers on position creation. * The fee is charged on the full notional value of the underlying's cash value - i.e. on 100%. - * Example: If providerLocked is 100, and callsTrike is 110%, the notional is 1000. If the APR is 1%, + * Example: If providerLocked is 100, and callStrike is 110%, the notional is 1000. If the APR is 1%, * so the fee on 1 year duration will be 1% of 1000 = 10, **on top** of the providerLocked. * So when position is created, the offer amount will be reduced by 100 + 10 in this example, * with 100 in providerLocked, and 10 sent to protocol fee recipient. @@ -162,7 +162,7 @@ contract CollarProviderNFT is ICollarProviderNFT, BaseNFT { fee = providerLocked * BIPS_BASE * feeAPR * duration / ((callStrikePercent - BIPS_BASE) * BIPS_BASE * YEAR) 4. Simplify BIPS_BASE: - fee = providerLocked * feeAPR * duration / ((callStrikePercent - BIPS_BASE) * YEAR + fee = providerLocked * feeAPR * duration / ((callStrikePercent - BIPS_BASE) * YEAR) */ // rounds up to prevent avoiding fee using many small positions. diff --git a/src/CollarTakerNFT.sol b/src/CollarTakerNFT.sol index e1d21371..287eaafe 100644 --- a/src/CollarTakerNFT.sol +++ b/src/CollarTakerNFT.sol @@ -86,7 +86,9 @@ contract CollarTakerNFT is ICollarTakerNFT, BaseNFT, ReentrancyGuard { TakerPositionStored memory stored = positions[takerId]; // do not try to call non-existent provider require(address(stored.providerNFT) != address(0), "taker: position does not exist"); - // @dev the provider position fields that are used are assumed to be immutable (set once) + // @dev the provider position fields used here are assumed to be immutable (set once). + // @dev a provider contract that manipulates these values should only be able to affect positions + // that are paired with it, but no other positions or funds. ICollarProviderNFT.ProviderPosition memory providerPos = stored.providerNFT.getPosition(stored.providerId); return TakerPosition({ diff --git a/src/ConfigHub.sol b/src/ConfigHub.sol index b3d721eb..41133792 100644 --- a/src/ConfigHub.sol +++ b/src/ConfigHub.sol @@ -15,14 +15,12 @@ import { IConfigHub } from "./interfaces/IConfigHub.sol"; * 1. Manages system-wide configuration and intern-contract authorization for the Collar Protocol. * 2. Controls which contracts are allowed to open positions for specific asset pairs. * 3. Sets valid ranges for key parameters like LTV and position duration. - * 4. Designates pause guardians who can pause contracts in an emergency. - * 5. Manages protocol fee parameters. + * 4. Manages protocol fee parameters. * * Post-Deployment Configuration: * - This Contract: Set valid LTV range for protocol * - This Contract: Set valid collar duration range for protocol * - This Contract: Set protocol fee parameters if fees are used - * - This Contract: Set pause guardians if using guardian functionality */ contract ConfigHub is Ownable2Step, IConfigHub { using EnumerableSet for EnumerableSet.AddressSet; @@ -109,8 +107,6 @@ contract ConfigHub is Ownable2Step, IConfigHub { emit CollarDurationRangeSet(min, max); } - // pausing - // protocol fee /// @notice Sets the APR in BIPs, and the address that receive the protocol fee. @@ -127,12 +123,14 @@ contract ConfigHub is Ownable2Step, IConfigHub { // ----- Views ----- /// @notice main auth for system contracts calling each other during opening of positions for a pair. - function canOpenPair(address underlying, address cashAsset, address target) + /// @dev note that assets A/B aren't necessarily external ERC20, and can be internal contracts as well + /// e.g, escrow may query (underlying, escrow, loans) to check a loans contract is enabled for it + function canOpenPair(address assetA, address assetB, address target) external view returns (bool) { - return canOpenSets[underlying][cashAsset].contains(target); + return canOpenSets[assetA][assetB].contains(target); } /// @notice equivalent to `canOpenPair` view when the second asset is ANY_ASSET placeholder diff --git a/src/base/BaseManaged.sol b/src/base/BaseManaged.sol index 51cda3f3..edc571e3 100644 --- a/src/base/BaseManaged.sol +++ b/src/base/BaseManaged.sol @@ -25,7 +25,7 @@ abstract contract BaseManaged { // ----- Modifiers ----- // modifier onlyConfigHubOwner() { - require(msg.sender == configHub.owner(), "BaseManaged: not configHub owner"); + require(msg.sender == configHubOwner(), "BaseManaged: not configHub owner"); _; } From 75fbbe23378cf8e9db02a814d3ee4823c0a5b1ff Mon Sep 17 00:00:00 2001 From: Arthur Deygin <29574203+artdgn@users.noreply.github.com> Date: Thu, 24 Apr 2025 09:52:32 +1000 Subject: [PATCH 12/12] Base mainnet deployment 0.3.0 Apr 2025 (#149) * update deployed artifacts * add the old artifacts file * add back no-deploy tests for new deployment * batch diff script * minor script improvements * udpate address regex to work in both bash and zsh --------- Co-authored-by: Kevin Utoft --- ...net_collar_protocol_deployment-latest.json | 23 ++++++++ ...net_collar_protocol_deployment-latest.json | 30 +++++------ script/utils/batch-diff-etherscan.sh | 54 +++++++++++++++++++ .../full-protocol/Loans.fork.opbase.t.sol | 4 -- 4 files changed, 92 insertions(+), 19 deletions(-) create mode 100644 script/artifacts/8453/DEPRECATED-opbase_mainnet_collar_protocol_deployment-latest.json create mode 100644 script/utils/batch-diff-etherscan.sh diff --git a/script/artifacts/8453/DEPRECATED-opbase_mainnet_collar_protocol_deployment-latest.json b/script/artifacts/8453/DEPRECATED-opbase_mainnet_collar_protocol_deployment-latest.json new file mode 100644 index 00000000..08ec718d --- /dev/null +++ b/script/artifacts/8453/DEPRECATED-opbase_mainnet_collar_protocol_deployment-latest.json @@ -0,0 +1,23 @@ +{ + "configHub": "0x230158944A372dC8801DC112Ac565F59579E6b4e", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_takerNFT": "0x68C5A88111b4d300734dBAECE7b16b809E712263", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_providerNFT": "0x179ef7D08416CBeE440b50e63deEbc0b40770df3", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_loansContract": "0x737837BCEA91Bb5b248bceF7A5Af3fCFD782B865", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_escrowSupplierNFT": "0x8d1081e8A6E5c29Ec3E6bDFE4D09a622ef22c369", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_rollsContract": "0x183C1bd07ab423A779F419A197618f94fAf8Efe7", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_oracle": "0x6e084fEF7bbcd123Ed1932C2E89D8ABa7bB74BE4", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapperUniV3": "0xCA8C969218c29A6A2F732E0a27a4EeB9217b318B", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_cashAsset": "0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_underlying": "0x4200000000000000000000000000000000000006", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapFeeTier": 500, + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_takerNFT": "0x28aff0dd8bb96E6cF4551bB1159B70746e84c072", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_providerNFT": "0x9180d9CF00b772EA4CAB31e3b86886b561B3dd44", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_loansContract": "0x117135AE96C46Fea950a487af224797011236609", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_escrowSupplierNFT": "0xA6b0D40e218E29BA626eAD3da4E8F146027A802D", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_rollsContract": "0xD243Ee8a96f56F0F28300aEba4e0D811f7550E16", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_oracle": "0x688D5D1bBb6C9476ce9E838c78dE43DB9AB5E232", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapperUniV3": "0x125A3CE7eB6a67ea29bC1E92Ce2b81962c4946dF", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_cashAsset": "0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_underlying": "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapFeeTier": 500 +} diff --git a/script/artifacts/8453/opbase_mainnet_collar_protocol_deployment-latest.json b/script/artifacts/8453/opbase_mainnet_collar_protocol_deployment-latest.json index fd206ca5..d5954e82 100644 --- a/script/artifacts/8453/opbase_mainnet_collar_protocol_deployment-latest.json +++ b/script/artifacts/8453/opbase_mainnet_collar_protocol_deployment-latest.json @@ -1,22 +1,22 @@ { - "configHub": "0x230158944A372dC8801DC112Ac565F59579E6b4e", - "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_takerNFT": "0x68C5A88111b4d300734dBAECE7b16b809E712263", - "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_providerNFT": "0x179ef7D08416CBeE440b50e63deEbc0b40770df3", - "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_loansContract": "0x737837BCEA91Bb5b248bceF7A5Af3fCFD782B865", - "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_escrowSupplierNFT": "0x8d1081e8A6E5c29Ec3E6bDFE4D09a622ef22c369", - "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_rollsContract": "0x183C1bd07ab423A779F419A197618f94fAf8Efe7", - "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_oracle": "0x6e084fEF7bbcd123Ed1932C2E89D8ABa7bB74BE4", - "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapperUniV3": "0xCA8C969218c29A6A2F732E0a27a4EeB9217b318B", + "configHub": "0x9dF9982B320bBEf6930289E325B08be52B0da8Bb", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_takerNFT": "0x674c357A26731874D3c1eAF2C00A1df4e0410121", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_providerNFT": "0xdbC703F1DF19eC3f0A43461C84a8C31Db3C07b13", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_loansContract": "0x149cee65F43913f21F2B72b4D45206fDE957E5E2", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_escrowSupplierNFT": "0xF2c42a0707927d6582072aEAb7AcB8A700455676", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_rollsContract": "0x5D189602FE891F56f00FA363BE1401291B4eb4CE", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_oracle": "0xF8b51d10307f4076d43eD8646dFF0144AdBFd5b9", + "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapperUniV3": "0x982214c1dEbABb12BA81a4CCe55412d0c50bAd34", "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_cashAsset": "0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913", "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_underlying": "0x4200000000000000000000000000000000000006", "0x4200000000000000000000000000000000000006_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapFeeTier": 500, - "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_takerNFT": "0x28aff0dd8bb96E6cF4551bB1159B70746e84c072", - "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_providerNFT": "0x9180d9CF00b772EA4CAB31e3b86886b561B3dd44", - "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_loansContract": "0x117135AE96C46Fea950a487af224797011236609", - "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_escrowSupplierNFT": "0xA6b0D40e218E29BA626eAD3da4E8F146027A802D", - "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_rollsContract": "0xD243Ee8a96f56F0F28300aEba4e0D811f7550E16", - "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_oracle": "0x688D5D1bBb6C9476ce9E838c78dE43DB9AB5E232", - "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapperUniV3": "0x125A3CE7eB6a67ea29bC1E92Ce2b81962c4946dF", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_takerNFT": "0x3Ec73f92aFE1F1FA862fA2d877e730221DF8065e", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_providerNFT": "0xB560c3A66e0af8B08a4e5A290F8ea651bf9Dda4b", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_loansContract": "0x67828bc77A84D12FbC8F0174797F2Aa9f2766cE8", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_escrowSupplierNFT": "0x87C127D4413e67d38C25b543cF8fC2c4a5f2fBC3", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_rollsContract": "0x9c7B346f85993F55c6fED828Cbcb93882B87060E", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_oracle": "0xF286396153348974df4e7166619823e0230FF1EA", + "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapperUniV3": "0x8886F99A7f38C4c3D1DbE48dAC62B3F2e33EC82f", "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_cashAsset": "0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913", "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_underlying": "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf", "0xcbB7C0000aB88B473b1f5aFd9ef808440eed33Bf_0x833589fCD6eDb6E08f4c7C32D4f71b54bdA02913_swapFeeTier": 500 diff --git a/script/utils/batch-diff-etherscan.sh b/script/utils/batch-diff-etherscan.sh new file mode 100644 index 00000000..49bf0815 --- /dev/null +++ b/script/utils/batch-diff-etherscan.sh @@ -0,0 +1,54 @@ +#!/bin/bash + +# chain ID is hardcoded +CHAIN_ID=8453 # base +# CHAIN_ID=84532 # base-sep + +echo "=== Batch Contract Diff ===" +echo "Using Chain ID: $CHAIN_ID (change in script as needed)" +echo "1. Paste your contract table below (copied from raw or rendered markdown). ContractName assumed to be last column." +echo "2. After pasting, press Enter" +echo "3. Then press Ctrl+D to finish input :" +# Create a temporary file to store the pasted table +TEMP_FILE=$(mktemp) +cat > "$TEMP_FILE" + +# Initialize counter for commands run +COMMANDS_RUN=0 + +# Process each line of the file +while IFS= read -r line; do + # Only process lines that contain /address/ + if [[ "$line" == *"/address/"* ]]; then + # Extract the address using regex + if [[ "$line" =~ (0x[a-fA-F0-9]{40}) ]]; then + # Extract the address using grep (works in both bash and zsh) + ADDRESS=$(grep -o '0x[a-fA-F0-9]\{40\}' <<< "$line") + + # Extract the contract name based on format + if [[ "$line" == *"|"* ]]; then + # For pipe-delimited markdown format + CONTRACT_NAME=$(echo "$line" | awk -F'|' '{print $(NF-1)}' | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') + else + # For tab-delimited rendered format + CONTRACT_NAME=$(echo "$line" | awk '{print $NF}' | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') + fi + + if [ -n "$ADDRESS" ] && [ -n "$CONTRACT_NAME" ]; then + # Construct and execute the command + COMMAND=". ./script/utils/diff-etherscan.sh $CHAIN_ID $ADDRESS src/$CONTRACT_NAME.sol" + echo "Running: $COMMAND" + eval "$COMMAND" + echo "--------------------------------------" + + # Increment the counter + ((COMMANDS_RUN++)) + fi + fi + fi +done < "$TEMP_FILE" + +echo "Total commands run: $COMMANDS_RUN" + +# Clean up the temporary file +rm -f "$TEMP_FILE" diff --git a/test/integration/full-protocol/Loans.fork.opbase.t.sol b/test/integration/full-protocol/Loans.fork.opbase.t.sol index a932a6fb..160c2a1c 100644 --- a/test/integration/full-protocol/Loans.fork.opbase.t.sol +++ b/test/integration/full-protocol/Loans.fork.opbase.t.sol @@ -126,8 +126,6 @@ contract WETHUSDC_OPBaseMain_LoansForkTest_NoDeploy is WETHUSDC_OPBaseMain_Loans super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; - // TODO: enable test when new release is deployed - vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseMain_artifactsName; } } @@ -157,8 +155,6 @@ contract CBBTCUSDC_OPBaseMain_LoansForkTest_NoDeploy is CBBTCUSDC_OPBaseMain_Loa super._setTestValues(); forceLatestBlock = true; testMode = TestMode.noDeploy; - // TODO: enable test when new release is deployed - vm.skip(true); // deployment is outdated (0.2.0) realArtifactsName = Const.OPBaseMain_artifactsName; } }