Omniscia Arcade XYZ Audit
OriginationController Manual Review Findings
OriginationController Manual Review Findings
OCR-01M: On-Chain Race Condition of Loan Consumptions
| Type | Severity | Location |
|---|---|---|
| Language Specific |  | OriginationController.sol:L757, L762 |
Description:
The OriginationController::_validateCounterparties function is meant to validate a loan's lender and borrower agreements to the loan's conditions, however, it may perform this validation using ECDSA signatures that are not attached to the caller.
As such, it is possible for a loan agreed by two parties to be "hijacked" by a secondary party that detects the transaction and submits their own with a higher fee in what is known as an on-chain race condition.
Impact:
The severity of this exhibit will be adjusted depending on whether the Arcade XYZ team wishes only direct lender-borrower relations, optional direct lender-borrower relations, or neither of the aforementioned functionalities.
Example:
738function _validateCounterparties(739 address borrower,740 address lender,741 address caller,742 address signer,743 Signature calldata sig,744 bytes32 sighash,745 Side neededSide746) internal view {747 address signingCounterparty = neededSide == Side.LEND ? lender : borrower;748 address callingCounterparty = neededSide == Side.LEND ? borrower : lender;749
750 // Make sure the signer recovered from the loan terms is not the caller,751 // and even if the caller is approved, the caller is not the signing counterparty752 if (caller == signer || caller == signingCounterparty) revert OC_ApprovedOwnLoan(caller);753
754 // Check that caller can actually call this function - neededSide assignment755 // defaults to BORROW if the signature is not approved by the borrower, but it could756 // also not be a participant757 if (!isSelfOrApproved(callingCounterparty, caller) && !isApprovedForContract(callingCounterparty, sig, sighash)) {758 revert OC_CallerNotParticipant(msg.sender);759 }760
761 // Check signature validity762 if (!isSelfOrApproved(signingCounterparty, signer) && !isApprovedForContract(signingCounterparty, sig, sighash)) {763 revert OC_InvalidSignature(signingCounterparty, signer);764 }765
766 // Revert if the signer is the calling counterparty767 if (signer == callingCounterparty) revert OC_SideMismatch(signer);768}Recommendation:
As this may be a desirable trait, we advise the Arcade XYZ team to evaluate the impact of this exhibit and either acknowledge it or carry out appropriate action by associating a lender and a borrower on-chain.
We advise that "private" loans between lenders and borrowers directly are facilitated by the contract optionally via an additional signature field as a potential remediation to this exhibit.
Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):
The Arcade XYZ team stated that they are aware of the potential to front-run a pending loan, however, they deem it inconsequential as there is no harmful economic impact to either counterparty of a processed loan.
Additionally, private loans can be facilitated between members by utilizing custom verifiers and / or EIP-1271 smart contracts that validate both counterparty addresses. As such, we consider this exhibit nullified.
OCR-02M: Restrictive Minimum Loan Principal
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | OriginationController.sol:L683 |
Description:
The MIN_LOAN_PRINCIPAL value is set as 1_000_000 irrespective of the actual decimals of the principal asset, causing the code to be incompatible with tokens such as WBTC.
Impact:
The OriginationController and thus entry-point of the Arcade XYZ lending and borrowing system is inherently incompatible with low-decimal high-value assets as the minimum enforced would introduce a significantly high bar of entry.
Example:
681function _validateLoanTerms(LoanLibrary.LoanTerms memory terms) internal virtual view {682 // principal must be greater than or equal to the MIN_LOAN_PRINCIPAL constant683 if (terms.principal < MIN_LOAN_PRINCIPAL) revert OC_PrincipalTooLow(terms.principal);684
685 // loan duration must be greater or equal to 1 hr and less or equal to 3 years686 if (terms.durationSecs < 3600 || terms.durationSecs > 94_608_000) revert OC_LoanDuration(terms.durationSecs);687
688 // interest rate must be greater than or equal to 0.01%689 // and less or equal to 10,000% (1e6 basis points)690 if (terms.proratedInterestRate < 1e18 || terms.proratedInterestRate > 1e24) revert OC_InterestRate(terms.proratedInterestRate);691
692 // signature must not have already expired693 if (terms.deadline < block.timestamp) revert OC_SignatureIsExpired(terms.deadline);694
695 // validate payable currency696 if (!allowedCurrencies[terms.payableCurrency]) revert OC_InvalidCurrency(terms.payableCurrency);697
698 // validate collateral699 if (!allowedCollateral[terms.collateralAddress]) revert OC_InvalidCollateral(terms.collateralAddress);700}Recommendation:
We advise the code to either enforce a USD based minimum loan principal (i.e. by utilizing Chainlink oracles), or to enforce a minimum loan principal per allowedCurrencies type, either of which we consider an adequate resolution to this exhibit.
Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):
Minimum loan principals are now dictated by dedicated entries per allowed currency, alleviating this exhibit in full and rendering the system compatible with any-decimal token.
OCR-03M: Improper Validation of Loan Amounts
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | OriginationController.sol:L508, L549, L863-L864, L867-L868 |
Description:
The OriginationController::recoverTokenSignature and OriginationController::recoverItemsSignature functions are meant to allow the borrower and lender of a loan to validate that they agree to the terms of a loan, however, the fees imposed on the loan's principal amount are not accounted for in these validations.
As a result, a borrower may have agreed to receive i.e. 1_000_000 USDC but would in reality receive less than that, significantly affecting the expectations of users when utilizing the Arcade XYZ platform.
Impact:
Users will sign payloads expecting to receive and provide a specific amount of the payable currency, however, the fees applied by the Arcade XYZ protocol will not be accounted for in these amounts and can additionally change between a transaction's submission and a transaction's execution by the network, further affecting the lender's and the borrower's user experience.
Example:
485/**486 * @notice Determine the external signer for a signature specifying only a collateral address and ID.487 *488 * @param loanTerms The terms of the loan.489 * @param sig The signature, with v, r, s fields.490 * @param nonce The signature nonce.491 * @param side The side of the loan being signed.492 *493 * @return sighash The hash that was signed.494 * @return signer The address of the recovered signer.495 */496function recoverTokenSignature(497 LoanLibrary.LoanTerms calldata loanTerms,498 Signature calldata sig,499 uint160 nonce,500 Side side501) public view override returns (bytes32 sighash, address signer) {502 bytes32 loanHash = keccak256(503 abi.encode(504 _TOKEN_ID_TYPEHASH,505 loanTerms.durationSecs,506 loanTerms.deadline,507 loanTerms.proratedInterestRate,508 loanTerms.principal,509 loanTerms.collateralAddress,510 loanTerms.collateralId,511 loanTerms.payableCurrency,512 loanTerms.affiliateCode,513 nonce,514 uint8(side)515 )516 );517
518 sighash = _hashTypedDataV4(loanHash);519 signer = ECDSA.recover(sighash, sig.v, sig.r, sig.s);520}521
522/**523 * @notice Determine the external signer for a signature specifying specific items.524 * @dev Bundle ID should _not_ be included in this signature, because the loan525 * can be initiated with any arbitrary bundle - as long as the bundle contains the items.526 *527 * @param loanTerms The terms of the loan.528 * @param sig The loan terms signature, with v, r, s fields.529 * @param nonce The signature nonce.530 * @param side The side of the loan being signed.531 * @param itemsHash The required items in the specified bundle.532 *533 * @return sighash The hash that was signed.534 * @return signer The address of the recovered signer.535 */536function recoverItemsSignature(537 LoanLibrary.LoanTerms calldata loanTerms,538 Signature calldata sig,539 uint160 nonce,540 Side side,541 bytes32 itemsHash542) public view override returns (bytes32 sighash, address signer) {543 bytes32 loanHash = keccak256(544 abi.encode(545 _ITEMS_TYPEHASH,546 loanTerms.durationSecs,547 loanTerms.deadline,548 loanTerms.proratedInterestRate,549 loanTerms.principal,550 loanTerms.collateralAddress,551 itemsHash,552 loanTerms.payableCurrency,553 loanTerms.affiliateCode,554 nonce,555 uint8(side)556 )557 );558
559 sighash = _hashTypedDataV4(loanHash);560 signer = ECDSA.recover(sighash, sig.v, sig.r, sig.s);561}Recommendation:
We advise the signature verification code to either utilize the "final" principal amounts for the lender and borrower or the fees the loan will utilize to be included in the signed payload, either of which we consider an adequate resolution to this exhibit.
Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):
The Arcade XYZ team weighed the code adjustments required for proper alleviation of this exhibit and concluded that they would significantly overhaul the semantics and logic of the contract in relation to the referenced signature recovery mechanisms.
Based on the perceived seldom occurrence of a fee change in a production environment, the Arcade XYZ team opted to acknowledge the exhibit and advise users to utilize counter-measures such as invalidated nonces as well as expiry timestamps that are in sync with the time required for a fee change to be processed.
OCR-04M: Insufficient Validation of Loan Rollover
| Type | Severity | Location |
|---|---|---|
| Logical Fault | | OriginationController.sol:L709-L723 |
Description:
The OriginationController::_validateRollover function will not validate whether a loan has expired, permitting expired loans to be rolled over improperly.
Impact:
It is possible for a user to rollover their loan even after it has expired, introducing similar discrepant behaviours to the repayments on expired loans as described in the relevant exhibit of RepaymentController.
Example:
709function _validateRollover(LoanLibrary.LoanTerms memory oldTerms, LoanLibrary.LoanTerms memory newTerms)710 internal711 pure712{713 if (newTerms.payableCurrency != oldTerms.payableCurrency)714 revert OC_RolloverCurrencyMismatch(oldTerms.payableCurrency, newTerms.payableCurrency);715
716 if (newTerms.collateralAddress != oldTerms.collateralAddress || newTerms.collateralId != oldTerms.collateralId)717 revert OC_RolloverCollateralMismatch(718 oldTerms.collateralAddress,719 oldTerms.collateralId,720 newTerms.collateralAddress,721 newTerms.collateralId722 );723}Recommendation:
We advise the code to ensure that a loan has not expired by validating the dueDate of a loan as calculated within LoanCore::claim, disallowing a loan from being possible to rollover when it has defaulted.
Alternatively, the rollover could be permitted solely if the lender remains the same if the loan has defaulted as this would essentially mean that the lender and borrower renegotiated their terms.
Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):
The Arcade XYZ team evaluated this exhibit and stated that they wish to leave the onus of liquidation / default to the loan provider akin to other lending protocols such as Aave, meaning that a loan can still be rolled over even if it has expired.
As such, we consider this exhibit nullified given that it describes desirable behaviour by the Arcade XYZ team.
OCR-05M: Invalidation of Predicates
| Type | Severity | Location |
|---|---|---|
| Logical Fault |  | OriginationController.sol:L225, L406 |
Description:
The predicates of a loan's initialization are evaluated before the LoanCore::_mintLoanNotes function is executed. As the creation flow of LoanCore will mint the relevant notes and then proceed to execute the relevant NFT transfers, the predicates can be invalidated by a re-entrant call during the creation of a loan note as a PromissoryNote::mint operation relies on ERC721::_safeMint which is re-entrant.
Impact:
Any predicate evaluated by the OriginationController::_runPredicatesCheck can be trivially invalidated by the borrower as their PromissoryNote is minted before the AssetVault is transferred, exposing even OwnableERC721::onlyOwner functionality to them before the asset is transferred.
Example:
200function initializeLoanWithItems(201 LoanLibrary.LoanTerms calldata loanTerms,202 address borrower,203 address lender,204 Signature calldata sig,205 uint160 nonce,206 LoanLibrary.Predicate[] calldata itemPredicates207) public override returns (uint256 loanId) {208 _validateLoanTerms(loanTerms);209 if (itemPredicates.length == 0) revert OC_PredicatesArrayEmpty();210
211 bytes32 encodedPredicates = _encodePredicates(itemPredicates);212
213 // Determine if signature needs to be on the borrow or lend side214 Side neededSide = isSelfOrApproved(borrower, msg.sender) ? Side.LEND : Side.BORROW;215
216 (bytes32 sighash, address externalSigner) = recoverItemsSignature(217 loanTerms,218 sig,219 nonce,220 neededSide,221 encodedPredicates222 );223
224 _validateCounterparties(borrower, lender, msg.sender, externalSigner, sig, sighash, neededSide);225 _runPredicatesCheck(borrower, lender, loanTerms, itemPredicates);226
227 loanCore.consumeNonce(externalSigner, nonce);228 loanId = _initialize(loanTerms, borrower, lender);229}Recommendation:
We advise either the PromissoryNote::mint function to be updated to utilize the ERC721::_mint operation over the ERC721::_safeMint operation, or the code to evaluate the predicates after the LoanCore contract has acquired custody of the AssetVault.
To note, all predicates would still be invalidated by lingering approval operations, however, this is covered in a separate exhibit within CallWhitelistApprovals.
Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):
The OriginationController::_runPredicatesCheck function invocations were relocated after the initialization and rollover of a loan, ensuring that the vault is properly in escrow when the predicates are run and thus preventing re-entrancies from exploiting this attack vector.