-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cross-Currency Rollover #133
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some preliminary thoughts after reviewing:
- I don’t love the inheritance of the OC. This would mean we have to deploy the OCMigrate and OCRollover separately and would require separate approvals from Lenders. Something we have tried to get away from in the past. I kind of like combining the CrossCurrencyRollover and OriginationControllerMigrate contracts to a single contract called OriginationControllerExtensions. There is a fair amount of overlap and could share logic related to custom errors, terms validations, sig validations, flash loan logic, initialize loan logic, etc..
- What is the reasoning behind fetching/calculating the current new asset price? This could lead to MEV manipulation since the minimum amount out is not specified. Take a look on Solodit and search slippage protection some can be highs. Additionally, removing this would remove a ton of logic and we wouldn't need all the extra dependancies, Solady, etc… Then just swap based on the price, if it is too high/low then you can deal with those outcomes directly. Lastly, I would also be in favor of whitelisting certain uniswap pools based on the
allowedPayableCurrency
mapping. This way the calculations are more predictable and wouldn't open up the case where someone can create a malicious pool with a whitelisted currency and create unexpected behavior. - Why in the setup scripts is loanCore.renounceRole moved to the end? Was this a result of the OCRollover deployment flow?
- In
_rollover()
curious why doesn't this account for the scenario where the new principal is greater than the old repayment amount? What param in the test can I tweak to verify this?NEW_PRINCIPAL
?
Users cannot create malicious UniswapV3 pools because the pool contracts are developed and deployed by Uniswap itself. To deploy a pool, users must use the Uniswap factory, which strictly controls the creation process. The options that users can manipulate, such as setting pool fees or providing initial liquidity, are predefined by Uniswap.
|
…ipal < repayment amount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic looks correct, and the necessary code is all there, but it's a bit spaghetti-ish. Left a comment about code organization.
…and with same lender
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more code clean up. Will review again once these are fixed.
CCR_ZeroAddress | ||
} from "../errors/Rollover.sol"; | ||
|
||
contract CrossCurrencyRollover is ICrossCurrencyRollover, OriginationController, ERC721Holder { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for only noticing this now, but I see we are doing a full inherit of OriginationController - this would mean that this contract could start regular loans too, not only cross-currency rollovers. I don't think this is ideal, since it eliminates the security/safety benefit from having rollovers in a separate contract from normal loan origination. If we give ORIGINATOR_ROLE
to this contract, we are giving it the power to do all forms of origination this contract supports, and right now this contract supports normal loan origination (not just rollovers).
I see that these are the functions you "need" from OriginationController
:
isApproved
isApprovedForContract
_recoverSignature
Maybe it's better to move all the functions in OC under PERMISSION MANAGEMENT
and SIGNATURE VERIFICATION
to a new contract, something like OriginationControllerBase
? Then both the original OC and CrossCurrencyRollover
can both inherit the base contract, but the "core origination" and "CC rollover" functionality is separate and not in the same contract (the same fix is also needed for OriginationControllerMigrate
, but that can be done separately from CC rollovers). Then you can also separate roles such that ROLLOVER_MANAGER_ROLE
isn't in the OriginationController
(where it's not needed).
// amount = original loan repayment amount - leftover principal | ||
uint256 amount = amounts.amountFromLender - amounts.leftoverPrincipal; | ||
|
||
if (swappedAmount > amount) { | ||
uint256 remainingFunds = swappedAmount - amount; | ||
|
||
// if funds remain, send to the borrower | ||
IERC20(oldLoanData.terms.payableCurrency).safeTransfer(borrower, remainingFunds); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't know what these lines are doing - is there a better variable name than amount
?
The comment also confuses things: why does original loan repayment amount
describe amounts.amountFromLender
? amounts.amountFromLender
is the principal for the new loan, not the loan repayment amount.
I get what you want to do here, you want to calculate if funds are leftover, and if so, send them to the borrower. But the name of amounts.amountToBorrower
suggests exactly that concept, no? We designed rolloverAmounts
to calculate exactly things like this (what funds need to go where for settlement), but now we are not using the outputs of that function, instead we're using swappedAmount
, but that was already an input for rolloverAmounts
calculation? Since the swap is already done, rolloverAmounts
should have all the information it needs.
So, the variable naming, usage of variables, and comments still don't quite clearly describe what is going on here. And as regards the accounting itself, if we break down the math in OriginationCalculator
, we can see that:
amounts.amountFromLender == newPrincipalAmount
- For the case where the borrower has funds leftover, so
repayAmount < newPrincipalAmount
, thenamounts.leftoverPrincipal == newPrincipalAmount + totalFees - repayAmount
So, amounts.amountFromLender - amounts.leftoverPrincipal
is equivalent to newPrincipalAmount - newPrincipalAmount + totalFees - repayAmount
, which is equivalent to totalFees - repayAmount
. Is this actually what we want to send to the borrower?
I recommend writing a test script where fees are turned onto some reasonable value (e.g. a 10% interest fee). I think the accounting might become more clear if we had those turned on. And my intuition is that, if we need to figure out what to send to the borrower, we should use amounts.amountToBorrower
. It's what it's named for, after all.
…dd script with fees
…parate out functionality
…ity for ocmigrate
* signature verification functions. | ||
* | ||
*/ | ||
contract OriginationControllerBase is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this should be abstract
OC_ApprovedOwnLoan, | ||
OC_InvalidSignature, | ||
OC_CallerNotParticipant, | ||
OC_SideMismatch, | ||
OC_RolloverCurrencyMismatch, | ||
OC_RolloverCollateralMismatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove unused imports in this file
bytes32 public constant MIGRATION_MANAGER_ROLE = keccak256("MIGRATION_MANAGER"); | ||
bytes32 public constant ROLLOVER_MANAGER_ROLE = keccak256("ROLLOVER_MANAGER"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove roles from the base contract, and put them in the relevant contracts (e.g. only CrossCurrencyRollover
needs to know about/use the ROLLOVER_MANAGER_ROLE
). As above, it doesn't look like OriginationControllerBase
needs AccessControl
at all, the child contracts do
FeeLookups, | ||
EIP712, | ||
OriginationCalculator, | ||
ReentrancyGuard, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider inherited contracts, it doesn't look like FeeLookups
, ReentrancyGuard
, or AccessControlEnumerable
are/should be used in this contract
ReentrancyGuard, | ||
AccessControlEnumerable | ||
{ | ||
contract OriginationController is IOriginationController, OriginationControllerBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should inherit FeeLookups
, ReentrancyGuard
, and AccessControlEnumerable
|
||
// ============================================ STATE ============================================== | ||
// ============== Constants ============== | ||
uint256 public constant ONE = 1e18; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove constant, not being used
{ | ||
(bytes32 sighash, address externalSigner) = _recoverSignature(newTerms, sig, sigProperties, Side.LEND, lender, itemPredicates); | ||
|
||
// counterparty validation | ||
if (!isSelfOrApproved(lender, externalSigner) && !OriginationLibrary.isApprovedForContract(lender, sig, sighash)) { | ||
revert CCR_SideMismatch(externalSigner); | ||
} | ||
|
||
// new lender cannot be the same as the borrower | ||
if (msg.sender == lender) revert CCR_LenderIsBorrower(); | ||
|
||
// consume new loan nonce | ||
loanCore.consumeNonce(externalSigner, sigProperties.nonce, sigProperties.maxUses); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove block scoping, things compile fine without it
IERC20(oldLoanData.terms.payableCurrency).safeTransferFrom(borrower, address(this), amounts.needFromBorrower); | ||
} | ||
|
||
// borrower get more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"gets" more?
// transfer collateral to LoanCore | ||
IERC721(newTerms.collateralAddress).transferFrom(address(this), address(loanCore), newTerms.collateralId); | ||
|
||
LoanLibrary.FeeSnapshot memory feeSnapshot = LoanLibrary.FeeSnapshot({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You create the feeSnapshot
struct twice, in _processSettlement
and here, meaning you are using double the memory. Have _processSettlement
return the struct, and then add it as a parameter to _initializeRolloverLoan
"Original lender DAI balance after rollover: ", | ||
ethers.utils.formatUnits(originalLenderDAIBalanceAfterRollover, 18), | ||
); | ||
console.log("New lender wETH balance is reduced correctly: ", isNewLenderBalanceCorrect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check LoanCore
balance to make sure fee was kept
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to look at the slippage math, but in the meantime I left some comments on the scripts. I still think the mainnet fork scripts are very rigid and dont allow for testing the broad range of possible new principal amounts. I left some comments on how to achieve this. I can also hop on a call to better explain if not clear.
contracts/errors/Rollover.sol
Outdated
* | ||
* This file contains custom errors for the cross currency rollover contract, with errors | ||
* prefixed by "CCR_" for CrossCurrencyRollover. | ||
* Errors located in one place to make it possible to holistically look at all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Errors located in one place to make it possible to holistically look at all protocol failure cases" doesnt hold true since this is separate from all the other lending errors. What is the reasoning for making them separate? All of the other origination controller flavors are in the lending.sol file.
scripts/deploy/deploy.ts
Outdated
libraries: { | ||
OriginationLibrary: originationLibrary.address, | ||
}, | ||
const OriginationControllerFactory = await ethers.getContractFactory("OriginationController", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont want a separate OC, the OC migrate is the one that should be deploy since it inherits the OC contract. This way lenders only need to make one approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, KvK made this same comment above.
@@ -216,6 +259,15 @@ export async function main(): Promise<DeployedResources> { | |||
loanCore: [borrowerNote.address, lenderNote.address], | |||
repaymentController: [loanCore.address, feeController.address], | |||
originationController: [originationHelpers.address, loanCore.address, feeController.address], | |||
originationControllerMigrate: [originationHelpers.address, loanCore.address, feeController.address], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove originationControllerMigrate
from here and the DeployedResources
struct. Notice in the record-deployemnt.ts
file there is no OriginationControllerMigrate. This is because the OrigiationController that is in that function is the OriginationControllerMigrate contract. In the current deployment script the origination controller is deployed as OriginationControllerMigrate. See the code below I copied from the current deploy script:
const OriginationControllerFactory = await ethers.getContractFactory("OriginationControllerMigrate",
{
libraries: {
OriginationLibrary: originationLibrary.address,
},
},
);
const originationController = <OriginationControllerMigrate>(
await OriginationControllerFactory.deploy(originationHelpers.address, loanCore.address, feeController.address)
);
await originationController.deployed();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, made this change for KvK's comment above.
const price = BigNumber.from("0x018974567f22d0"); | ||
console.log("Price of DAI in wETH at block number: 18852467: ", ethers.utils.formatUnits(price, 18)); | ||
// changing the value of NEW_PRINCIPAL will affect whether the borrower will | ||
// need to provide additional funds to pay the difference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is the case if the new principal value is less than the old pricipal value. It would be nice to be able to change this so that all three scenarions are covered.
- old principal > new pricipal
- old principal = new pricipal
- old principal < new pricipal
My suggestion would be to hardcode what ever the equivlent value of WETH is for 3000 DAI, then you know if you make it higher or lower it will cover all three cases. The .div(2)
seems very arbitrary.
console.log("Approvals for rollover loan..."); | ||
|
||
// new lender approves WETH amount to contract | ||
const approveWETHTx = await weth.connect(newLender).approve(crossCurrencyRollover.address, wethAmount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should approve the exact amount (NEW_PRINCIPAL) for testing best practice.
); | ||
|
||
const swapParams: SwapParameters = { | ||
minAmountOut: amountOwed.div(2), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the constant, so there is no math here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of small optimizations, then looks good to me
// calculate the repay amount to settle the original loan | ||
repayAmount = oldLoanData.terms.principal + interestAmount; | ||
|
||
LoanLibrary.FeeSnapshot memory feeSnapshot = LoanLibrary.FeeSnapshot({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we need to create new feeSnapshot in memory here. Can use oldLoanData.lenderInterestFee
and oldLoanData.lenderPrincipalFee
in the rolloverAmounts call. Also seems best to remove it from here and _executeRollover()
. Create it in _initializeRolloverLoan()
since that is the only place it is needed. and your loading the oldLoanData
there anyways. Will save some gas
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, this creates a new memory slot, different from the feeSnapshot
that is intended to be returned from the function as declared on line 317
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion.
newLoanTerms.principal, | ||
swapParams.minAmountOut, | ||
swapParams.poolFeeTier, | ||
address(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just hardcode address(this)
in _swapExactInputSingle()
less code
uint256 oldLoanId, | ||
LoanLibrary.FeeSnapshot memory feeSnapshot | ||
) internal nonReentrant returns (uint256 newLoanId) { | ||
LoanLibrary.LoanData memory oldLoanData = ILoanCore(loanCore).getLoan(oldLoanId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already get the oldLoanData in rolloverCrossCurrencyLoan()
can it be passed as input to this function? Will save an external call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not even used in this function, so it can just be deleted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
// calculate the repay amount to settle the original loan | ||
repayAmount = oldLoanData.terms.principal + interestAmount; | ||
|
||
LoanLibrary.FeeSnapshot memory feeSnapshot = LoanLibrary.FeeSnapshot({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, this creates a new memory slot, different from the feeSnapshot
that is intended to be returned from the function as declared on line 317
uint256 oldLoanId, | ||
LoanLibrary.FeeSnapshot memory feeSnapshot | ||
) internal nonReentrant returns (uint256 newLoanId) { | ||
LoanLibrary.LoanData memory oldLoanData = ILoanCore(loanCore).getLoan(oldLoanId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not even used in this function, so it can just be deleted here.
@@ -269,7 +279,7 @@ contract OriginationControllerMigrate is IMigrationBase, OriginationController, | |||
// calculate the repay amount to settle V3 loan | |||
repayAmount = oldLoanData.terms.principal + interest; | |||
|
|||
amounts = rolloverAmounts( | |||
amounts = OriginationCalculator.rolloverAmounts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remnant from a previous iteration.
LoanLibrary.LoanTerms memory newLoanTerms, | ||
LoanLibrary.LoanData memory oldLoanData, | ||
uint256 oldLoanId | ||
) internal nonReentrant returns (uint256 repayAmount){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting, missing a space:
) internal nonReentrant returns (uint256 repayAmount){ | |
) internal nonReentrant returns (uint256 repayAmount) { |
// calculate the repay amount to settle the original loan | ||
repayAmount = oldLoanData.terms.principal + interestAmount; | ||
|
||
OriginationLibrary.RolloverAmounts memory amounts = OriginationCalculator.rolloverAmounts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed
OriginationLibrary.RolloverAmounts memory amounts = OriginationCalculator.rolloverAmounts( | |
OriginationLibrary.RolloverAmounts memory amounts = rolloverAmounts( |
// borrower owes | ||
if (amounts.needFromBorrower > 0) { | ||
IERC20(oldLoanData.terms.payableCurrency).safeTransferFrom(borrower, address(this), amounts.needFromBorrower); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the following clause can be linked via if-else if
, right? Since there's no possible way that both amounts.needFromBorrower > 0
AND amounts.amountToBorrower > 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100%
|
||
address oldLender = ILoanCore(loanCore).lenderNote().ownerOf(oldLoanId); | ||
|
||
// calculate settle amounts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Settlement amounts"? We should try to keep comments as readable as possible, and grammatical correctness helps w/ readability
|
||
// ====================================== CURRENCY MIGRATION ============================================ | ||
/** | ||
* @notice Migrate an active loan on from one currency to another. This function validates new loan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Migrate" is still used here in the comment, and there is a typo (extra "on")
* @notice Migrate an active loan on from one currency to another. This function validates new loan | |
* @notice Roll over an active loan from one currency to another. This function validates new loan |
bytes32 public constant ROLLOVER_MANAGER_ROLE = keccak256("ROLLOVER_MANAGER"); | ||
|
||
/// @notice UniswapV3Factory | ||
address private constant POOL_FACTORY = 0x1F98431c8aD98523631AE4a59f267346ea31F984; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale for having POOL_FACTORY
be an constant but swapRouter
be an immutable
? Seems like it creates a vector to confuse things (if you deploy with a swapRouter
reference that doesn't actually match this factory).
It looks like the PeripheryImmutableState contract (which SwapRouter
inherits) contains its own reference to a factory. I think, you should pull the factory from the swap router at constructor time, to make sure they are always correctly "paired". Up to you whether you want to make the swap router address constant
(like the POOL_FACTORY
is now), or leave it immutable
and set in the constructor. The latter always makes testing easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a great point.
I removed the POOL_FACTORY
reference from the contract. That was only needed for function fetchCurrentPrice()
which I had in the previous iteration of the contract.
* @notice Validates that the rollover is valid. If any of these conditionals are not met | ||
* the transaction will revert. | ||
* | ||
* @dev All whitelisted payable currencies and collateral must be whitelisted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not actually checking whitelisting in this function (originationHelpers.validateLoanTerms
does that), so maybe remove this comment
* @param sig The signature of the loan terms. | ||
* @param sigProperties The properties of the signature. | ||
* @param itemPredicates The predicates for the loan. | ||
* @param swapParams The parameters for the currency swap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param swapParams The parameters for the currency swap. | |
* @param swapParams The fee tier and slippage tolerance for the currency swap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much better.
@@ -41,6 +41,11 @@ library OriginationLibrary { | |||
RolloverAmounts migrationAmounts; | |||
} | |||
|
|||
struct SwapParameters { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be part of ICrossCurrencyRollover
, since it is only used in the cross-currency case
This pull request introduces a feature that allows users to rollover loans from one currency to another, provided the new currency is whitelisted by the lending protocol.
It integrates
UniswapV3
functionalities by importing from@uniswap/v3-periphery
and@uniswap/v3-core
.Due to compiler version compatibility issues,
UniswapV3
'sPoolAddress.sol
library has been included locally and updated topragma solidity 0.8.18
.To test on a fork of mainnet, run:
FORK_MAINNET=true npx hardhat run scripts/rollover/cross-currency-rollover-new-lender.ts
using block number
18852467
.To test scenario where a loan is rolled over to a new currency where the lender for the original loan and the new loan is the same address, run:
FORK_MAINNET=true npx hardhat run scripts/rollover/cross-currency-rollover-same-lender.ts
using block number
18852467
.