From ceb3a6888cdb4258c916f2185bee18c131fdab70 Mon Sep 17 00:00:00 2001 From: asardon Date: Thu, 10 Aug 2023 17:54:08 +0200 Subject: [PATCH] added fixes for statemind-171, 173 and 175 --- contracts/peer-to-peer/AddressRegistry.sol | 1 - contracts/peer-to-peer/LenderVaultImpl.sol | 6 +- .../interfaces/IAddressRegistry.sol | 6 -- .../interfaces/ILenderVaultImpl.sol | 12 +-- .../BasicQuotePolicyManager.sol | 16 ++- .../policyManagers/DataTypesBasicPolicies.sol | 3 - test/peer-to-peer/helpers/misc.ts | 5 +- test/peer-to-peer/local-tests.ts | 100 +++++++----------- test/peer-to-peer/mainnet-forked-tests.ts | 41 ++++--- 9 files changed, 77 insertions(+), 113 deletions(-) diff --git a/contracts/peer-to-peer/AddressRegistry.sol b/contracts/peer-to-peer/AddressRegistry.sol index 6dd2400b..8f282c2c 100644 --- a/contracts/peer-to-peer/AddressRegistry.sol +++ b/contracts/peer-to-peer/AddressRegistry.sol @@ -27,7 +27,6 @@ contract AddressRegistry is Initializable, Ownable2Step, IAddressRegistry { address public borrowerGateway; address public quoteHandler; address public mysoTokenManager; - address public quotePolicyManager; address public erc721Wrapper; address public erc20Wrapper; mapping(address => bool) public isRegisteredVault; diff --git a/contracts/peer-to-peer/LenderVaultImpl.sol b/contracts/peer-to-peer/LenderVaultImpl.sol index d5683b71..da1fdc7e 100644 --- a/contracts/peer-to-peer/LenderVaultImpl.sol +++ b/contracts/peer-to-peer/LenderVaultImpl.sol @@ -414,10 +414,6 @@ contract LenderVaultImpl is return _loans.length; } - function totalNumSigners() external view returns (uint256) { - return signers.length; - } - function getTokenBalancesAndLockedAmounts( address[] calldata tokens ) @@ -443,7 +439,7 @@ contract LenderVaultImpl is } } - function numSigners() external view returns (uint256) { + function totalNumSigners() external view returns (uint256) { return signers.length; } diff --git a/contracts/peer-to-peer/interfaces/IAddressRegistry.sol b/contracts/peer-to-peer/interfaces/IAddressRegistry.sol index c4e3adc3..c330154a 100644 --- a/contracts/peer-to-peer/interfaces/IAddressRegistry.sol +++ b/contracts/peer-to-peer/interfaces/IAddressRegistry.sol @@ -186,12 +186,6 @@ interface IAddressRegistry { */ function mysoTokenManager() external view returns (address); - /** - * @notice Returns the address of the quote policy manager - * @return Address of the quote policy manager contract - */ - function quotePolicyManager() external view returns (address); - /** * @notice Returns boolean flag indicating whether given address is a registered vault * @param addr Address to check if it is a registered vault diff --git a/contracts/peer-to-peer/interfaces/ILenderVaultImpl.sol b/contracts/peer-to-peer/interfaces/ILenderVaultImpl.sol index 5ea60307..6fbe259e 100644 --- a/contracts/peer-to-peer/interfaces/ILenderVaultImpl.sol +++ b/contracts/peer-to-peer/interfaces/ILenderVaultImpl.sol @@ -7,7 +7,7 @@ import {DataTypesPeerToPeer} from "../DataTypesPeerToPeer.sol"; interface ILenderVaultImpl { event AddedSigners(address[] _signers); - event MinNumberOfSignersSet(uint256 numSigners); + event MinNumberOfSignersSet(uint256 minNumSigners); event RemovedSigner( address signerRemoved, @@ -237,10 +237,10 @@ interface ILenderVaultImpl { function pendingOwner() external view returns (address); /** - * @notice function to return number of signers + * @notice function to return the total number of signers * @return number of signers */ - function numSigners() external view returns (uint256); + function totalNumSigners() external view returns (uint256); /** * @notice function to return unlocked token balances @@ -316,10 +316,4 @@ interface ILenderVaultImpl { * @return total number of loans */ function totalNumLoans() external view returns (uint256); - - /** - * @notice function returns total number of signers - * @return total number of signers - */ - function totalNumSigners() external view returns (uint256); } diff --git a/contracts/peer-to-peer/policyManagers/BasicQuotePolicyManager.sol b/contracts/peer-to-peer/policyManagers/BasicQuotePolicyManager.sol index 493c852d..843a9f39 100644 --- a/contracts/peer-to-peer/policyManagers/BasicQuotePolicyManager.sol +++ b/contracts/peer-to-peer/policyManagers/BasicQuotePolicyManager.sol @@ -26,6 +26,9 @@ contract BasicQuotePolicyManager is IQuotePolicyManager { addressRegistry = _addressRegistry; } + // @dev: When no global policy is set (default case), all pairs are automatically blocked except + // for those where a pair policy is explicitly set. In the case where a global policy is set, + // all pairs are assumed to be allowed (no blocking). function setGlobalPolicy( address lenderVault, bytes calldata globalPolicyData @@ -42,7 +45,6 @@ contract BasicQuotePolicyManager is IQuotePolicyManager { DataTypesBasicPolicies.GlobalPolicy memory currGlobalPolicy = _globalQuotingPolicies[lenderVault]; if ( - globalPolicy.allowAllPairs == currGlobalPolicy.allowAllPairs && globalPolicy.requiresOracle == currGlobalPolicy.requiresOracle && _equalQuoteBounds( @@ -67,6 +69,9 @@ contract BasicQuotePolicyManager is IQuotePolicyManager { emit GlobalPolicySet(lenderVault, globalPolicyData); } + // @dev: If no global policy is set, then setting a pair policy allows one to explicitly unblock a specific pair; + // in the other case where a global policy is set, setting a pair policy allows overwriting global policy + // parameters as well as overwriting minimum signer threshold requirements. function setPairPolicy( address lenderVault, address collToken, @@ -135,7 +140,7 @@ contract BasicQuotePolicyManager is IQuotePolicyManager { bool hasPairPolicy = _hasPairQuotingPolicy[lenderVault][ generalQuoteInfo.collToken ][generalQuoteInfo.loanToken]; - if (!globalPolicy.allowAllPairs && !hasPairPolicy) { + if (!_hasGlobalQuotingPolicy[lenderVault] && !hasPairPolicy) { return (false, 0); } @@ -294,10 +299,11 @@ contract BasicQuotePolicyManager is IQuotePolicyManager { ) { return false; } - } - if (quoteTuple.upfrontFeePctInBase < quoteBounds.minFee) { - return false; + // @dev: only check upfront fee for loans (can skip for swaps where tenor=0) + if (quoteTuple.upfrontFeePctInBase < quoteBounds.minFee) { + return false; + } } return true; diff --git a/contracts/peer-to-peer/policyManagers/DataTypesBasicPolicies.sol b/contracts/peer-to-peer/policyManagers/DataTypesBasicPolicies.sol index cd2028ce..07a39e91 100644 --- a/contracts/peer-to-peer/policyManagers/DataTypesBasicPolicies.sol +++ b/contracts/peer-to-peer/policyManagers/DataTypesBasicPolicies.sol @@ -20,9 +20,6 @@ library DataTypesBasicPolicies { } struct GlobalPolicy { - // Flag indicating if all pairs are allowed (=true) or - // per default only pairs with explicitly defined pair policy (=false) - bool allowAllPairs; // Flag indicating if an oracle is required for the pair bool requiresOracle; // Applicable global bounds diff --git a/test/peer-to-peer/helpers/misc.ts b/test/peer-to-peer/helpers/misc.ts index 54a02cea..28d324da 100644 --- a/test/peer-to-peer/helpers/misc.ts +++ b/test/peer-to-peer/helpers/misc.ts @@ -335,14 +335,13 @@ export type QuoteBounds = { maxLoanPerCollUnitOrLtv: BigNumber } -export const encodeGlobalPolicy = (allowAllPairs: boolean, requiresOracle: boolean, quoteBounds: QuoteBounds): string => { +export const encodeGlobalPolicy = (requiresOracle: boolean, quoteBounds: QuoteBounds): string => { return ethers.utils.defaultAbiCoder.encode( [ - 'bool allowAllPairs', 'bool requiresOracle', 'tuple(uint32 minTenor, uint32 maxTenor, uint80 minFee, int80 minApr, uint32 minEarliestRepayTenor, uint128 minLoanPerCollUnitOrLtv, uint128 maxLoanPerCollUnitOrLtv) quoteBounds' ], - [allowAllPairs, requiresOracle, quoteBounds] + [requiresOracle, quoteBounds] ) } diff --git a/test/peer-to-peer/local-tests.ts b/test/peer-to-peer/local-tests.ts index a792937e..f6374087 100644 --- a/test/peer-to-peer/local-tests.ts +++ b/test/peer-to-peer/local-tests.ts @@ -1807,11 +1807,10 @@ describe('Peer-to-Peer: Local Tests', function () { ) // reverts when trying to remove with signer idx that is out-of-bounds - let numSigners = Number((await lenderVault.numSigners()).toString()) - await expect(lenderVault.connect(lender).removeSigner(borrower.address, numSigners)).to.be.revertedWithCustomError( - lenderVault, - 'InvalidArrayIndex' - ) + let totalNumSigners = Number((await lenderVault.totalNumSigners()).toString()) + await expect( + lenderVault.connect(lender).removeSigner(borrower.address, totalNumSigners) + ).to.be.revertedWithCustomError(lenderVault, 'InvalidArrayIndex') // reverts when trying to remove unknown signer address await expect(lenderVault.connect(lender).removeSigner(weth.address, 0)).to.be.revertedWithCustomError( @@ -1830,10 +1829,10 @@ describe('Peer-to-Peer: Local Tests', function () { await lenderVault.connect(lender).removeSigner(signerAddr, 0) // remove all signers (always remove last one to test edge case handling) - numSigners = Number((await lenderVault.numSigners()).toString()) - for (let i = 0; i < numSigners; i++) { - const lastSignerAddr = await lenderVault.signers(numSigners - i - 1) - await lenderVault.connect(lender).removeSigner(lastSignerAddr, numSigners - i - 1) + totalNumSigners = Number((await lenderVault.totalNumSigners()).toString()) + for (let i = 0; i < totalNumSigners; i++) { + const lastSignerAddr = await lenderVault.signers(totalNumSigners - i - 1) + await lenderVault.connect(lender).removeSigner(lastSignerAddr, totalNumSigners - i - 1) } // set borrower whitelist @@ -6012,7 +6011,7 @@ describe('Peer-to-Peer: Local Tests', function () { maxLoanPerCollUnitOrLtv: BASE.div(2) } - const globalPolicyData = encodeGlobalPolicy(true, false, quoteBounds) + const globalPolicyData = encodeGlobalPolicy(false, quoteBounds) const pairPolicyData = encodePairPolicy(true, 1, quoteBounds) @@ -6084,30 +6083,22 @@ describe('Peer-to-Peer: Local Tests', function () { await expect( basicPolicyManager .connect(lender) - .setGlobalPolicy( - lenderVault.address, - encodeGlobalPolicy(true, false, { ...quoteBounds, minTenor: ONE_DAY.mul(40) }) - ) + .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(false, { ...quoteBounds, minTenor: ONE_DAY.mul(40) })) ).to.be.revertedWithCustomError(basicPolicyManager, 'InvalidTenors') await expect( basicPolicyManager .connect(lender) - .setGlobalPolicy( - lenderVault.address, - encodeGlobalPolicy(true, false, { ...quoteBounds, minLoanPerCollUnitOrLtv: BASE }) - ) + .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(false, { ...quoteBounds, minLoanPerCollUnitOrLtv: BASE })) ).to.be.revertedWithCustomError(basicPolicyManager, 'InvalidLoanPerCollOrLtv') await expect( basicPolicyManager .connect(lender) - .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(true, false, { ...quoteBounds, minApr: BASE.mul(-2) })) + .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(false, { ...quoteBounds, minApr: BASE.mul(-2) })) ).to.be.revertedWithCustomError(basicPolicyManager, 'InvalidMinApr') - await basicPolicyManager - .connect(lender) - .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(true, true, quoteBounds)) + await basicPolicyManager.connect(lender).setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(true, quoteBounds)) // borrow should fail without oracle address await expect( @@ -6118,7 +6109,7 @@ describe('Peer-to-Peer: Local Tests', function () { await basicPolicyManager .connect(lender) - .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(true, false, { ...quoteBounds, minTenor: ONE_DAY.mul(35) })) + .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(false, { ...quoteBounds, minTenor: ONE_DAY.mul(35) })) // borrow fail if tenor less than min tenor await expect( @@ -6129,7 +6120,7 @@ describe('Peer-to-Peer: Local Tests', function () { await basicPolicyManager .connect(lender) - .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(true, false, { ...quoteBounds, maxTenor: ONE_DAY.mul(25) })) + .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(false, { ...quoteBounds, maxTenor: ONE_DAY.mul(25) })) // borrow fail if tenor greater than max tenor await expect( @@ -6140,10 +6131,7 @@ describe('Peer-to-Peer: Local Tests', function () { await basicPolicyManager .connect(lender) - .setGlobalPolicy( - lenderVault.address, - encodeGlobalPolicy(true, false, { ...quoteBounds, minFee: BASE.mul(9).div(10) }) - ) + .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(false, { ...quoteBounds, minFee: BASE.mul(9).div(10) })) // borrow should fail if upfront fee is below min fee await expect( @@ -6154,7 +6142,7 @@ describe('Peer-to-Peer: Local Tests', function () { await basicPolicyManager .connect(lender) - .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(true, false, { ...quoteBounds, minApr: BASE.mul(10) })) + .setGlobalPolicy(lenderVault.address, encodeGlobalPolicy(false, { ...quoteBounds, minApr: BASE.mul(10) })) // borrow should fail if apr is below min apr await expect( @@ -6163,32 +6151,28 @@ describe('Peer-to-Peer: Local Tests', function () { .borrowWithOnChainQuote(lenderVault.address, borrowInstructions1, onChainQuote1, quoteTupleIdx1) ).to.be.revertedWithCustomError(quoteHandler, 'QuoteViolatesPolicy') - await basicPolicyManager - .connect(lender) - .setGlobalPolicy( - lenderVault.address, - encodeGlobalPolicy(true, false, { - ...quoteBounds, - minLoanPerCollUnitOrLtv: BASE.mul(4).div(5), - maxLoanPerCollUnitOrLtv: BASE.mul(9).div(10) - }) - ) + await basicPolicyManager.connect(lender).setGlobalPolicy( + lenderVault.address, + encodeGlobalPolicy(false, { + ...quoteBounds, + minLoanPerCollUnitOrLtv: BASE.mul(4).div(5), + maxLoanPerCollUnitOrLtv: BASE.mul(9).div(10) + }) + ) // borrow should go through even if loanPerCollUnitOrLtv is below minLoanPerCollUnitOrLtv if no oracle required await borrowerGateway .connect(borrower) .borrowWithOnChainQuote(lenderVault.address, borrowInstructions1, onChainQuote1, quoteTupleIdx1) - await basicPolicyManager - .connect(lender) - .setGlobalPolicy( - lenderVault.address, - encodeGlobalPolicy(true, false, { - ...quoteBounds, - minLoanPerCollUnitOrLtv: ethers.BigNumber.from(0), - maxLoanPerCollUnitOrLtv: ethers.BigNumber.from(0) - }) - ) + await basicPolicyManager.connect(lender).setGlobalPolicy( + lenderVault.address, + encodeGlobalPolicy(false, { + ...quoteBounds, + minLoanPerCollUnitOrLtv: ethers.BigNumber.from(0), + maxLoanPerCollUnitOrLtv: ethers.BigNumber.from(0) + }) + ) // borrow should go through even if loanPerCollUnitOrLtv is above maxLoanPerCollUnitOrLtv if no oracle required await borrowerGateway @@ -6227,16 +6211,14 @@ describe('Peer-to-Peer: Local Tests', function () { 'OnChainQuoteAdded' ) - await basicPolicyManager - .connect(lender) - .setGlobalPolicy( - lenderVault.address, - encodeGlobalPolicy(true, false, { - ...quoteBounds, - minApr: BASE.mul(-99).div(100), - minEarliestRepayTenor: ONE_DAY.mul(20) - }) - ) + await basicPolicyManager.connect(lender).setGlobalPolicy( + lenderVault.address, + encodeGlobalPolicy(false, { + ...quoteBounds, + minApr: BASE.mul(-99).div(100), + minEarliestRepayTenor: ONE_DAY.mul(20) + }) + ) // borrow with negative interest rate should fail if earliest min repay is too short await expect( diff --git a/test/peer-to-peer/mainnet-forked-tests.ts b/test/peer-to-peer/mainnet-forked-tests.ts index 1a3231de..3b585db9 100644 --- a/test/peer-to-peer/mainnet-forked-tests.ts +++ b/test/peer-to-peer/mainnet-forked-tests.ts @@ -5332,29 +5332,14 @@ describe('Peer-to-Peer: Forked Mainnet Tests', function () { minLoanPerCollUnitOrLtv: BASE.mul(11).div(100), maxLoanPerCollUnitOrLtv: BASE.mul(11).div(100) } - let allowAllPairs = false let globalRequiresOracle = false - let globalPolicyData = encodeGlobalPolicy(allowAllPairs, globalRequiresOracle, globalQuoteBounds) + let globalPolicyData = encodeGlobalPolicy(globalRequiresOracle, globalQuoteBounds) // check revert when trying to delete global policy although not yet set await expect( basicQuotePolicyManager.connect(lender).setGlobalPolicy(lenderVault.address, '0x') ).to.be.revertedWithCustomError(basicQuotePolicyManager, 'NoPolicyToDelete') - // set global policy - await basicQuotePolicyManager.connect(lender).setGlobalPolicy(lenderVault.address, globalPolicyData) - expect(await basicQuotePolicyManager.hasGlobalQuotingPolicy(lenderVault.address)).to.be.true - const actGlobalPolicyData = await basicQuotePolicyManager.globalQuotingPolicy(lenderVault.address) - expect(actGlobalPolicyData.allowAllPairs).to.be.equal(allowAllPairs) - expect(actGlobalPolicyData.requiresOracle).to.be.equal(globalRequiresOracle) - expect(actGlobalPolicyData.quoteBounds.minTenor).to.be.equal(globalQuoteBounds.minTenor) - expect(actGlobalPolicyData.quoteBounds.maxTenor).to.be.equal(globalQuoteBounds.maxTenor) - expect(actGlobalPolicyData.quoteBounds.minFee).to.be.equal(globalQuoteBounds.minFee) - expect(actGlobalPolicyData.quoteBounds.minApr).to.be.equal(globalQuoteBounds.minApr) - expect(actGlobalPolicyData.quoteBounds.minEarliestRepayTenor).to.be.equal(globalQuoteBounds.minEarliestRepayTenor) - expect(actGlobalPolicyData.quoteBounds.minLoanPerCollUnitOrLtv).to.be.equal(globalQuoteBounds.minLoanPerCollUnitOrLtv) - expect(actGlobalPolicyData.quoteBounds.maxLoanPerCollUnitOrLtv).to.be.equal(globalQuoteBounds.maxLoanPerCollUnitOrLtv) - // lender produces off-chain quote let quoteTuples = [ { @@ -5432,18 +5417,30 @@ describe('Peer-to-Peer: Forked Mainnet Tests', function () { // borrower approves gateway await weth.connect(borrower).approve(borrowerGateway.address, MAX_UINT256) - // check revert if borrow violates policy (here because allow all pairs is false and no explicit pair policy set) + // check revert if borrow violates policy (here because no global policy is set in which case + // per default all pairs are blocked) await expect( borrowerGateway .connect(borrower) .borrowWithOffChainQuote(lenderVault.address, borrowInstructions, offChainQuote, selectedQuoteTuple, proof) ).to.be.revertedWithCustomError(quoteHandler, 'QuoteViolatesPolicy') - // update global policy to allow all pairs - allowAllPairs = true - globalPolicyData = encodeGlobalPolicy(allowAllPairs, globalRequiresOracle, globalQuoteBounds) + // add global policy to allow all pairs + globalPolicyData = encodeGlobalPolicy(globalRequiresOracle, globalQuoteBounds) await basicQuotePolicyManager.connect(lender).setGlobalPolicy(lenderVault.address, globalPolicyData) + // check global policy is set correctly + expect(await basicQuotePolicyManager.hasGlobalQuotingPolicy(lenderVault.address)).to.be.true + const actGlobalPolicyData = await basicQuotePolicyManager.globalQuotingPolicy(lenderVault.address) + expect(actGlobalPolicyData.requiresOracle).to.be.equal(globalRequiresOracle) + expect(actGlobalPolicyData.quoteBounds.minTenor).to.be.equal(globalQuoteBounds.minTenor) + expect(actGlobalPolicyData.quoteBounds.maxTenor).to.be.equal(globalQuoteBounds.maxTenor) + expect(actGlobalPolicyData.quoteBounds.minFee).to.be.equal(globalQuoteBounds.minFee) + expect(actGlobalPolicyData.quoteBounds.minApr).to.be.equal(globalQuoteBounds.minApr) + expect(actGlobalPolicyData.quoteBounds.minEarliestRepayTenor).to.be.equal(globalQuoteBounds.minEarliestRepayTenor) + expect(actGlobalPolicyData.quoteBounds.minLoanPerCollUnitOrLtv).to.be.equal(globalQuoteBounds.minLoanPerCollUnitOrLtv) + expect(actGlobalPolicyData.quoteBounds.maxLoanPerCollUnitOrLtv).to.be.equal(globalQuoteBounds.maxLoanPerCollUnitOrLtv) + // check revert if borrow violates policy (here because min signer threshold is breached) await expect( borrowerGateway @@ -5686,9 +5683,9 @@ describe('Peer-to-Peer: Forked Mainnet Tests', function () { // lenderVault owner deposits usdc await usdc.connect(lender).transfer(lenderVault.address, ONE_USDC.mul(100000)) - const preTotalNumSigners = await lenderVault.numSigners() + const preTotalNumSigners = await lenderVault.totalNumSigners() await lenderVault.connect(lender).addSigners([team.address]) - const postTotalNumSigners = await lenderVault.numSigners() + const postTotalNumSigners = await lenderVault.totalNumSigners() expect(postTotalNumSigners.sub(preTotalNumSigners)).to.be.equal(1) // deploy chainlinkOracleContract