-
Notifications
You must be signed in to change notification settings - Fork 192
[VEN-2895]: Flash loan implementation #545
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
base: feat/VEN-3361
Are you sure you want to change the base?
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.
We cannot modify this contract. vBNB is not upgradable
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, I know but vBNB is inheriting the vToken contract which is been modified so I had to modify the vBNB as well.
we can create a separate contract for the vToken
which would contain the old logic(without flashloan) and can inherit that one to the vBNB to maintain the consistency. let me know wdyt?
} | ||
|
||
// Call the execute operation on receiver contract | ||
if (!IFlashLoanReceiver(receiver).executeOperation(assets, amounts, fees, receiver, "")) { |
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 initiator could be a different address of receiver
. I think we should use msg.sender
instead. wdyt?
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.
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.
Sorry, I was meant the other way:
if (!IFlashLoanReceiver(receiver).executeOperation(assets, amounts, fees, msg.sender, param)) {
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.
Refactored.
contracts/Tokens/VTokens/VToken.sol
Outdated
* custom:access Only Governance | ||
* custom:event Emits FlashLoanFeeUpdated event on success | ||
*/ | ||
function _setFlashLoanFeeMantissa(uint256 fee) external returns (uint256) { |
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.
Shouldn't we have two fees? one for the protocol and another one for the suppliers?
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 guess we should have one fee for protocol, as suppliers are getting the interest rate by suppling the assets to the market. we can discuss this point.
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.
Let's say the flash-loan borrower asks for 100 tokens. With the current implementation (one fee), we calculate the fee (let's say 10) so the borrower has to return 110. With the current implementation, the fee (10) is "socialised" among the suppliers (the exchange rate has increased, because the available cash has increased after the flash loan).
I think the protocol reserve should receive part of those 10 tokens. Here is when a second fee value is needed. Let's say 50%. So, 5 tokens are for the suppliers (socialised via an increase in the available cash), and 5 tokens are for the PSR contract.
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.
} | ||
|
||
for (uint256 k; k < len; k++) { | ||
(assets[k]).verifyBalance(balanceBefore[k], amounts[k] + fees[k]); |
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.
Don't we have to transferFrom
first, to receive the tokens from the receiver? AFAIU, the receiver is not going to transfer the funds, they will only approve them
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.
Direct transfer
would be executed by the executeOperation
in FlashLoanSimpleReceiver
receiver contract in the current scenario. we can implement the approve
logic as well. let me know your thoughts
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 think we agreed on following the approve
approach
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.
|
||
/** | ||
* @notice Executes a flashLoan operation with the requested assets. | ||
* @dev Transfer the specified assets to the receiver contract and handles repayment. |
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.
* @dev Transfer the specified assets to the receiver contract and handles repayment. | |
* @dev Transfers the specified assets to the receiver contract and handles repayment. |
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.
VToken vToken, | ||
address payable initiator, | ||
address payable receiver, | ||
uint256 amountRepayed, |
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.
uint256 amountRepayed, | |
uint256 amountRepaid, |
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.
uint256 accrueResult = vToken.accrueInterest(); | ||
require(accrueResult == 0, "Failed to accrue interest"); | ||
|
||
uint256 debtError = vToken.borrowDebtPosition(initiator, leftUnpaidBalance); |
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.
Hm, I thought it was on the receiver's balance sheet. If not, I guess @Exef can avoid a borrow behalf in his code
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 were opening debt position for the user's EOA that executed flashLoan which is onBehalf
.
receiver
is the contract that receives flashloan amount . we can't open borrow position for a contract .
if (debtError != 0) { | ||
revert FailedToCreateDebtPosition(); | ||
} | ||
} else { |
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.
Shouldn't we transfer the fee regardless of whether it was a full repayment or not?
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 premiums The premiums (fees) associated with each flash-borrowed asset. | ||
* @param initiator The address that initiated the flashLoan operation. | ||
* @param param Additional parameters encoded as bytes. These can be used to pass custom data to the receiver contract. | ||
* @return True if the operation succeeds and the borrowed amount plus the premium is repaid, false otherwise. |
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 should document the second returned value as well
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.
contracts/Tokens/VTokens/VToken.sol
Outdated
* @custom:access Only Governance | ||
* @custom:event Emits ToggleFlashLoanEnabled event on success | ||
*/ | ||
function toggleFlashLoan() external returns (uint256) { |
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'd prefer to have a setter with an explicit true
or false
parameter. This way we can avoid race conditions between, e.g., a guardian transaction and a VIP transaction.
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.
contracts/Tokens/VTokens/VToken.sol
Outdated
ensureAdmin(msg.sender); | ||
require(accrualBlockNumber == 0 && borrowIndex == 0, "market may only be initialized once"); | ||
require( | ||
accrualBlockNumber == 0 && borrowIndex == 0 && (initialExchangeRateMantissa_ > 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.
Saving space?
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 used yarn prettier
.
contracts/Tokens/VTokens/VToken.sol
Outdated
} | ||
|
||
/* Revert if protocol has insufficient underlying cash */ | ||
if (getCashPrior() < borrowAmount) { |
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 don't need cash if we don't transfer anything
if (getCashPrior() < borrowAmount) { | |
if (shouldTransfer && getCashPrior() < borrowAmount) { |
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.
contracts/Tokens/VTokens/VToken.sol
Outdated
uint error = accrueInterest(); | ||
if (error != uint(Error.NO_ERROR)) { | ||
// accrueInterest emits logs on errors, but on top of that we want to log the fact that an attempted reserve factor change failed. | ||
return fail(Error(error), info); |
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.
With the proposed implementation this return value is ignored and operations continue as normal even if interest accruals fail. I don't think we want that.
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.
revert SenderNotAuthorizedForFlashLoan(msg.sender); | ||
} | ||
|
||
if (!approvedDelegates[onBehalf][msg.sender]) { |
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 think we can skip this if msg.sender == onBehalf
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.
msg.sender
can't be onBehalf
as msg.sender
will be the user's receiver contract and onBehalf
is necessary to get the actual user's address to open the borrow position in case of partial repayment .
* @author Venus | ||
*/ | ||
contract VBep20Delegator is VTokenInterface, VBep20Interface, VDelegatorInterface { | ||
abstract contract VBep20Delegator is VTokenInterface, VBep20Interface, VDelegatorInterface { |
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.
Shouldn't we implement borrowDebtPosition
in this contract, and delegate it to the implementation (using delegateToImplementation
), as we are doing for the rest of the functions?
address asset, | ||
address sender, |
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.
Indexed?
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.
/** | ||
* @notice open a debt position for the borrower | ||
* @dev This function checks if the borrow is allowed, accrues interest, and updates the borrower's balance. | ||
* It also emits a Borrow event and calls the comptroller's borrowVerify function. | ||
* It reverts if the borrow is not allowed, if the market's block number is not current, or if the protocol has insufficient cash. | ||
* @param borrower The address of the borrower | ||
* @param borrowAmount The amount of underlying asset to borrow | ||
* @return uint Returns 0 on success, otherwise returns a failure code (see ErrorReporter.sol for details). | ||
*/ | ||
function borrowDebtPosition(address borrower, uint borrowAmount) external returns (uint) { | ||
bytes memory data = delegateToImplementation( | ||
abi.encodeWithSignature("borrowDebtPosition(address,uint256)", borrower, borrowAmount) | ||
); | ||
return abi.decode(data, (uint)); | ||
} | ||
|
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 didn't expect this change in the R1 contract. We don't need it, right?
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.
contracts/Tokens/VTokens/VToken.sol
Outdated
uint allowed = comptroller.redeemAllowed(address(this), redeemer, vars.redeemTokens); | ||
if (allowed != 0) { | ||
revert("math error"); | ||
return failOpaque(Error.COMPTROLLER_REJECTION, FailureInfo.REDEEM_COMPTROLLER_REJECTION, allowed); |
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.
Why are we changing the revert
with a return
? External integrations can expect the revert in these cases, not checking the returned value. There are several instances of 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.
contracts/Tokens/VTokens/VToken.sol
Outdated
0 | ||
); | ||
} | ||
ensureNoMathError(vars.mathErr); |
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 wouldn't change the returned values, if it's possible. I would revert this change
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.
contracts/Tokens/VTokens/VToken.sol
Outdated
(uint repayBorrowError, uint actualRepayAmount) = repayBorrowFresh(liquidator, borrower, repayAmount); | ||
if (repayBorrowError != uint(Error.NO_ERROR)) { | ||
return (fail(Error(repayBorrowError), FailureInfo.LIQUIDATE_REPAY_BORROW_FRESH_FAILED), 0); | ||
uint err; |
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 understand this is to try to optimize the size of the contract. Now that we don't have the simple flash loan function, I suppose we can keep this as it was, to minimize the changes
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 will revert the changes. The function can be handled with a single err
variable, but I will maintain the original structure.
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.
Description
This PR introduces the implementation of flashloan functionality along with Partial Repayment for the Core pool on BNB Chain. Flashloan enable users to borrow assets without collateral, provided the loan is repaid within the same transaction. This enhancement includes the following changes:
a major new feature: multi-asset flash loan support in the Venus Protocol's Comptroller contract. It adds the necessary interfaces, storage, access control, and execution logic to enable secure and flexible flash loans, including whitelisting for authorized accounts and handling of fees and repayments. Several contracts and interfaces are updated to support this new functionality.
Flash Loan Feature Implementation
PolicyFacet
, including phased logic for fee calculation, asset transfer, receiver contract interaction, and repayment/fee handling. This includes checks for asset enablement, parameter validity, and sender authorization.IFlashLoanReceiver
,IFlashLoanSimpleReceiver
) to standardize interaction with contracts receiving flash-loaned assets.Storage and Interface Refactoring
ComptrollerV17Storage
toComptrollerV18Storage
to support flash loan data and authorization. Updated all relevant imports and contract inheritance.ComptrollerInterface
,IPolicyFacet
,ISetterFacet
) to expose new flash loan and whitelist functions.Repayment Options
Events and Observability
Access Control and Whitelisting
setWhiteListFlashLoanAccount
method inSetterFacet
, event emission, and corresponding interface updates.Interface and External Contract Support
IFlashLoanReceiver
andIFlashLoanSimpleReceiver
) to ensure contracts interacting with the flash loan system implement the required logic for asset repayment.NOTE : deployments needs to done with new contract changes .