V2ToV3RolloverWithItems Manual Review Findings

VTW-01M: Inexplicable Implementation of Receive Function

Type	Severity	Location
Code Style		V2ToV3RolloverWithItems.sol:L234

Description:

The V2ToV3RolloverWithItems::receive function's presence is inexplicable as the V2ToV3RolloverWithItems contract does not deal with native funds.

Impact:

The severity of this exhibit will be adjusted accordingly when the Arcade Protocol team has taken the correct course of action for it.

Example:

Copy
229borrowerNoteV3.safeTransferFrom(address(this), borrower, newLoanId);

Recommendation:

We advise the Arcade Protocol team to either elaborate its presence or to omit it, depending on the desired functionality of the V2ToV3RolloverWithItems contract.

Alleviation:

The V2ToV3RolloverWithItems::receive function has been safely omitted as advised, reducing the bytecode size of the contract.

VTW-02M: Potentially Out-of-Sync Literal

Type	Severity	Location
Standard Conformity		V2ToV3RolloverWithItems.sol:L142

Description:

The "BORROWER_ORIGINATION_FEE" string literal is meant to act as the key to access the borrower's loan origination fee, however, it may change throughout the lifetime of the FeeController contract.

Example:

Copy
139uint256(
140    feeControllerV3.getLendingFee(
141        // FL_01 - borrower origination fee
142        keccak256("BORROWER_ORIGINATION_FEE")
143    )
144),

Recommendation:

We advise the code to instead import the FeeLookups contract and utilize its FL_01 data entry, ensuring that the code of V2ToV3RolloverWithItems is properly synchronized with the keys present in and utilized by the FeeController.

Alleviation:

The borrower origination fee is adequately fetched using the FL_01 literal of FeeLookups in conjunction with the FeeController::getLendingFee function, standardizing the code's style and easing its maintainability.

VTW-03M: Improper Consumption of Flash-Loan Calls

Type	Severity	Location
Logical Fault		V2ToV3RolloverWithItems.sol:L104-L114

Description:

The V2ToV3RolloverWithItems::receiveFlashLoan contract will trust all calls made via the VAULT contract which may not have necessarily been initialized by the contract itself via V2ToV3RolloverWithItems::rolloverLoanWithItems. As such, a major vulnerability manifests in the way loans are rolled-over that can be exploited via an on-chain race condition.

To elaborate, the V2ToV3RolloverWithItems::receiveFlashLoan function will consume the alleged borrower's approvals to repay their old loan and open a new one. In its validation mechanism, it solely ensures that the NFT asset, ID of NFT, and payable currency are the same with no regard to the actual loan terms.

Given that the V2ToV3RolloverWithItems::_initializeNewLoanWithItems function will ultimately validate a lender rather than borrower signature as it creates a loan originating from the roll-over contract, it is possible for a malicious party to detect a loan rollover and "race" it with their own transaction which would cause the V2 loan to be repaid and a significantly unfavourable V3 loan to be opened with a different lender than intended (i.e. w/ a significantly high interest rate).

Impact:

It is presently possible to compromise loans that are being rolled over via on-chain race conditions, causing the underlying collateral of the loan as well as the additional funds the borrower needs to supply for the original loan to be repaid to be compromised.

Example:

Copy
104function receiveFlashLoan(
105    IERC20[] calldata assets,
106    uint256[] calldata amounts,
107    uint256[] calldata feeAmounts,
108    bytes calldata params
109) external nonReentrant {
110    if (msg.sender != address(VAULT)) revert R_UnknownCaller(msg.sender, address(VAULT));
111
112    OperationDataWithItems memory opData = abi.decode(params, (OperationDataWithItems));
113    _executeOperation(assets, amounts, feeAmounts, opData);
114}

Recommendation:

We advise the rollover workflow to be revised, caching the original caller in the V2ToV3RolloverWithItems::rolloverLoanWithItems function and validating them in the V2ToV3RolloverWithItems::receiveFlashLoan function. Such a security measure would guarantee that loans originate from the V2ToV3RolloverWithItems contract and thus that the validations performed by V2ToV3RolloverWithItems::_validateRollover are inherited by the V2ToV3RolloverWithItems::_executeOperation function properly.

Alleviation:

The V2ToV3RolloverBase dependency was updated to introduce a new modifier called V2ToV3RolloverBase::whenBorrowerReset which ensures that the newly declared storage variable borrower is zero at the beginning of the function's invocation and is cleared at the end.

This mechanism is utilized to store the borrower of the roll-over during a V2ToV3RolloverWithItems::rolloverLoanWithItems invocation that is consequently validated by the V2ToV3RolloverWithItems::receiveFlashLoan function. As such, the flash-loan callback hook can solely be invoked as part of a flash-loan originating from the contract itself alleviating this exhibit in full.