Omniscia Myso Finance Audit
LenderVaultImpl Manual Review Findings
LenderVaultImpl Manual Review Findings
LVI-01M: Potentially Dangerous Overwrite of Storage
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | LenderVaultImpl.sol:L110-L124 |
Description:
The LenderVaultImpl::updateLoanInfo function will overwrite any entry that was previously present in the _loans[loanId] entry with the input memory loan. This behaviour can lead to multiple types of attacks, including re-entrancy ones, that would affect the protocol significantly.
Impact:
The current function implementation would have been exploitable if the BorrowerGateway functions had not been marked as nonReentrant, showcasing that a potential update of the protocol could cause this vulnerability to manifest.
The adjustment proposed that removes attack vectors relying on re-entrancies and outdated memory data would also lead to a gas optimization of LenderVaultImpl::updateLoanInfo as it would require less input arguments. As such, we strongly advise the Myso Finance team to introduce this change even if there is no presently exploitable attack vector.
Example:
110function updateLoanInfo(111 DataTypesPeerToPeer.Loan memory _loan,112 uint128 repayAmount,113 uint256 loanId,114 uint256 collAmount115) external {116 senderCheckGateway();117 _loan.amountRepaidSoFar += repayAmount;118
119 // only update lockedAmounts when no compartment120 if (_loan.collTokenCompartmentAddr == address(0)) {121 lockedAmounts[_loan.collToken] -= collAmount;122 }123 _loans[loanId] = _loan;124}Recommendation:
Given that the _loan member passed into LenderVaultImpl::updateLoanInfo by BorrowerGateway::repay is not adjusted and is the same as the loan in the storage of LenderVaultImpl, we advise the amountRepaidSoFar member of _loans[loanId] to be adjusted directly. This would also render the entire _loan argument redundant, instead only requiring the collToken and collTokenCompartmentAddr members of the struct as new input arguments to LenderVaultImpl::updateLoanInfo.
Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):
The code now properly updates the amountRepaidSoFar member of the Loan struct in storage, ensuring that an outdated memory struct does not overwrite its storage and thus alleviating this exhibit in full.
LVI-02M: Unsafe Type Casts
| Type | Severity | Location |
|---|---|---|
| Mathematical Operations | | LenderVaultImpl.sol:L179, L180-L182 |
Description:
The referenced type casts to a uint40 are performed unsafely.
Impact:
While the type casts are performed unsafely, they would manifest as unwanted behaviour solely in the case the protocol applies a zero percent fee per annum.
In such a case, a quote could be created with an abnormally high tenor that would cause the block.timestamp + tenor calculation to be performed safely but truncate when cast to an uint40 variable.
Example:
179_loan.expiry = uint40(block.timestamp + quoteTuple.tenor);180_loan.earliestRepay = uint40(181 block.timestamp + generalQuoteInfo.earliestRepayTenor182);Recommendation:
We advise them to be performed safely, ensuring that the values being cast are at most equal to type(uint40).max, the maximum value an unsigned integer of 40 bits can hold.
Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):
All referenced type casts are now performed safely via the import of the SafeCast library from OpenZeppelin, alleviating this exhibit.
LVI-03M: Insufficient Restriction of Loan Tenors
| Type | Severity | Location |
|---|---|---|
| Input Sanitization |  | LenderVaultImpl.sol:L183 |
Description:
The LenderVaultImpl::processQuote function will validate that the expiry of a loan is greater than the earliestRepay, however, this delta can be as little as one second.
Impact:
Should the expiry be only one second away from the earliestRepay, the repayment of the loan would be impossible in practice due to the varying block times a blockchain has as the time window the transaction would need to execute in would be 1 second.
Example:
183if (_loan.expiry <= _loan.earliestRepay) {184 revert Errors.ExpiresBeforeRepayAllowed();185}Recommendation:
We advise a minimum time delta to be enforced between a loan's expiry and a loan's earliest repayment, ensuring that the purveyor of the loan has sufficient time to repay the loan under normal conditions (i.e. 24 hours).
Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):
A minimum delta of 24 hours has been introduced to the codebase in the form of Constants::MIN_TIME_BETWEEN_EARLIEST_REPAY_AND_EXPIRY, guaranteeing adequate time for a borrower to repay their loan and thus alleviating this exhibit.
LVI-04M: Incorrect Interest Rate Factor Validation
| Type | Severity | Location |
|---|---|---|
| Mathematical Operations |  | LenderVaultImpl.sol:L436 |
Description:
The interest rate factor percent that is permitted by LenderVaultImpl::getLoanAndRepayAmount is contradictory to the sanitization performed in QuoteHandler::isValidOnChainQuote as it permits the edge case of _interestRateFactor being 0.
In such a case, the repayment amount would become 0 thus disallowing the user from ever repaying their "-100% interest" loan due to a division by zero. As such, a malicious lender could create off-chain quotes of "-100%" interest loans that victims would be allured by and the lender would then have to wait until those loans expire to claim them as the users would never be able to repay them.
Impact:
It is currently possible for a malicious lender to create seemingly lucrative loans that will ultimately cause the users to lose their collateral and never be able to repay the loans. In practice, this would be exploited by creating an off-chain quote (which does not go through the QuoteHandler::isValidOnChainQuote process) that also utilizes a basic compartment (ensuring that LenderVaultImpl::unlockCollateral will not suffer from the same division-by-zero error). Users would be enticed to take out these loans as they would assume the lender made a mistake when in reality the lender crafted these loans maliciously.
Example:
434int256 _interestRateFactor = int256(Constants.BASE) +435 quoteTuple.interestRatePctInBase;436if (_interestRateFactor < 0) {437 revert Errors.NegativeRepaymentAmount();438}439uint256 interestRateFactor = uint256(_interestRateFactor);440repayAmount = (loanAmount * interestRateFactor) / Constants.BASE;Recommendation:
We strongly advise the LenderVaultImpl::getLoanAndRepayAmount function to be updated, yielding an error in case the _interestRateFactor amounts to 0 and thus prohibiting this edge case from manifesting.
Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):
The LenderVaultImpl::getLoanAndRepayAmount function has been updated as advised, preventing an _interestRateFactor of 0 from being specified and thus preventing this vulnerability from manifesting.
LVI-05M: Insecure Collateral Compartment Creation
| Type | Severity | Location |
|---|---|---|
| Logical Fault | | LenderVaultImpl.sol:L191-L194 |
Description:
The collateral compartments of the Myso Finance project are meant to support specific asset classes or even unique assets. Quotes validated by the QuoteHandler::checkSenderAndGeneralQuoteInfo as well as by the LenderVaultImpl::processQuote functions do not sanitize the type of compartment paired with a particular collateral asset.
This is a significant flaw as for example a quote that has specified the GLPStakingCompartment with a WETH collateral would successfully execute. In such a case, any withdrawal would be essentially "doubled" as the BaseCompartment::_transferCollFromCompartment function would unlock the proportionate withdrawal of WETH and the top-level GLPStakingCompartment::transferCollFromCompartment function would re-calculate and re-unlock another proportion of WETH, affecting the loan's repayment behaviour in favour of the borrower.
Impact:
As an example exploitation flow for unaware lenders, a borrower may ask a lender to utilize the CurveLPStakingCompartment for their CRV collateral. Such a quote would cause the double-withdraw exploit to manifest and seemingly appears "correct" as a Curve-related asset is utilized with a Curve-related compartment.
Example:
362function createCollCompartment(363 address borrowerCompartmentImplementation,364 uint256 loanId365) internal returns (address collCompartment) {366 if (367 IAddressRegistry(addressRegistry).whitelistState(368 borrowerCompartmentImplementation369 ) != DataTypesPeerToPeer.WhitelistState.COMPARTMENT370 ) {371 revert Errors.NonWhitelistedCompartment();372 }373 bytes32 salt = keccak256(374 abi.encodePacked(375 borrowerCompartmentImplementation,376 address(this),377 loanId378 )379 );380 collCompartment = Clones.cloneDeterministic(381 borrowerCompartmentImplementation,382 salt383 );384 IBaseCompartment(collCompartment).initialize(address(this), loanId);385}Recommendation:
We advise a new function to be introduced to the IBaseCompartment that is meant to be overridden by derivative implementations, is view, and validates its input address as being a supported collateral or not.
The code of LenderVaultImpl::createCollCompartment would then need to be updated to invoke this function after it has been created, ensuring that the created compartment explicitly supports the collateral that will be sent to it.
Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):
The Myso Finance team has opted to remediate this vulnerability using an alternative approach, introducing a new whitelist in the AddressRegistry that associates tokens with their compartments and making use of this whitelist for all types of quotes (off-chain and on-chain) at the QuoteHandler contract when they are created as well as consumed.
After reviewing the changes that were made to introduce this system, we consider this exhibit fully alleviated as the quote-based validation mechanism works as expected and will validate the compartment and token associations both at each quote's creation as well as consumption.