Arcade A-2

In SeaportBasicRoute.sol::_checkSuccess, the following check is made when ERC20 tokens are received from OpenSea call:

} else if (oType >= 4) {
        // Receiving ERC20
        if (
            IERC20(basicOrderParams.offerToken).balanceOf(address(this)) !=  
            (basicOrderParams.offerAmount - recipientsTotal)
        ) {
            revert OR_CallFailed(returnData);
        }
}

The transaction reverts if the balance of the contract doesn’t match with offerAmount - recipientsTotal. This can be exploited by a malicious user that sends an arbitrary amount of the respective ERC20 tokens directly to the contract’s address.

Remediations to Consider

Consider reverting only if contract doesn’t hold sufficient balance.

if (
    IERC20(basicOrderParams.offerToken).balanceOf(address(this)) <
    (basicOrderParams.offerAmount - recipientsTotal)
) {
    revert OR_CallFailed(returnData);
}

InCollateralSaleSeaportBasic.sol::fulfillCollateralSale(..), one can specify an ERC1155 token with specific amount as the vault item. However, the specific amount considered for the collateral sale is not taken into account when withdrawing the assets in CollateralSale.sol::_withdrawAssets:

...
else if (vaultItems[i].vaultItemType == LendingPlusLibrary.VaultItemType.ERC1155) {
    // send back 1155's to the seller
    assetVault.withdrawERC1155(vaultItems[i].tokenAddress, vaultItems[i].tokenId, recipient);

Instead of the specific consideration amount, the full amount of ERC1155 held by the vault is withdrawn to the contract’s address, but only the consideration amount is sold via OpenSea. As a consequence, the remaining amount of ERC1155 tokens are locked up in the contract.

Remediations to Consider

Consider only withdrawing the exact consideration amount to the contract’s address and transfer the remaining amount back to the borrower’s address.

The repayment functions in RepaymentCoordinator.sol and LoanCore.sol do not account for transfer tax, which results in less value transferred than requested. Loan payments are first transferred to RepaymentCoordinator.sol, then LoanCore.sol will re-send to the lender. When transfer tax is enabled on the payment currency, the value received by RepaymentCoordinator.sol is less than the expected amount, so the subsequent lender payment fails. As a result, the loan cannot be repaid, and may be forced into default. This costs borrowers any prior loan payments plus collateral.

Malicious ERC20 operators can exploit this by lending and then enabling transfer tax towards the end of an installment loan period. This allows them to recoup most of their capital, as well as claim collateral.

Remediations to Consider

Potential options may include:

• Implement a whitelist of supported payable currencies (e.g. no transfer tax implementation; not upgradeable; etc)
• Allowing borrowers to account for transfer tax in their payments. Note that borrowers will effectively be forced to over-pay the loan in this scenario.
• Consider adding documentation/guidance regarding the use of transfer-tax currencies.

ItemsVerifier.sol contains logic which restricts the valid range of token ids to [0, max(int256)], whereas valid token ids range from [0, max(uint256)]. As a result, an asset with token id > max(int256) cannot be explicitly collateralized for loan origination. This occurs because the SignatureItem struct defines tokenId as an int256 type, in order to treat negative values as wildcard.

Remediations to Consider

Consider updating the tokenId datatype to uint256, and implementing the wildcard option via a new struct value.

RepaymentController.sol::repayPart can be called multiple times within the same installment period. Due to the following logic in InstallmentsCalc::_calcAmountDue,

if (numInstallmentsPaid >= _installmentPeriod) {
  // When numInstallmentsPaid is greater than or equal to the _installmentPeriod
  // this indicates that the minimum interest and any late fees for this installment period
  // have already been repaid. Any additional amount sent in this installment period goes to principal
  return (0, 0, 0);
}

installment interest only has to be paid on 1st call, for any subsequent calls to repayPart, the _calcAmountDue function returns (0, 0, 0).

As a consequence, a borrower can call repayPart within 1st installment period and pays down the minimum payment for the first installment. Next, he can call repayPart n times (n stands for the number of installment periods) with amount of only 1 wei, thereby incrementing the numInstallmentsPaid variable in LoanCore::repayPart, which indicates subsequent installment periods as already paid. When the loan expires, the borrower only has to pay back the principal amount; thus avoiding all further installment interests and late fees that would have otherwise been accrued.

Consider the following scenario:

1. Installment loan is initialized with:
   • duration: 1 year, principal: 100 ether, interest 10%, number installments: 10
2. If borrower always pays in time (and thus no late fees accrue), the lender should receive 110 ethers at the end of the year, incl. the 10 ethers in interest
3. However, malicious borrower can do the following during 1st installment period:
   1. Calls repayPart with minimum payment of 1 ether
   2. Calls repayPart 10 times (as number of installments are 10) with amount of only 1 wei
4. At the end of the year, borrower only has to pay back the principal amount (minus 9 wei)
5. As a result, lender receives only 1 ether in interest instead of the expected 10 ethers.

Note that in the above scenario, the lender - in addition to loosing interests - can’t claim the collateral after 40% of missed payments, as all installment periods are indicated as paid.

Remediations to Consider

• Don’t allow small repayments, only allow repayments for amount >= minBalDue as calculated in InstallmentsCalc::_calcAmountsDue
• or calculate numInstallmentsPaid in LoanCore.repayPart based on current loan balance

ERC777 - which implements ERC20 and can be used as a payable currency - makes callbacks to the token receiver upon transfer. The receiver can force a revert, causing the transfer to fail.

If a loan is made with an ERC777 payable currency, the lender can choose whether to deny any repayments (e.g. the final installment) from the borrower, forcing a default and the loss of collateral.

Remediations to Consider

Potential options may include:

• Implement a whitelist of supported payable currencies.
• Upon origination check if the EIP-1820 registry has the payable currency registered as an ERC777Token interface implementer. Consider preventing the loan in this scenario.
• Upon repayment failure check if the EIP-1820 registry has the lender registered as an ERC777TokensRecipient interface implementer. Consider pausing the loan in this scenario to allow the lender to remove their registry entry: such that the lender can unpause when the registry entry is removed. Special considerations here may be necessary for:
  • An updated repayment timeline
  • Reasonable outcomes if the loan is never unpaused
• Update documentation to advise borrowers of these risks (i.e. caveat emptor).

The documentation mentions:

Transfer methods are blacklisted in order to prevent backdoor withdrawals from vaults.

CallWhitelist.sol explicitly blacklists transfer/approval functions for vault assets of type ERC20/ERC721/ERC1155, to create trust with users that these assets cannot be removed from collateralized vaults. However, certain functions are not accounted for in this blacklist:

• increaseAllowance() for ERC20.
• CryptoPunks have a unique set of functions to control transfer of ownership:
  • transferPunk()
  • offerPunkForSale()
  • offerPunkForSaleToAddress()
  • buyPunk()

This opens the potential that these methods could be whitelisted, which may create trust concerns for Arcade protocol users

Remediations to Consider

Consider adding the above methods to the blacklist

If a user sends a token directly to the PayLater contract, all payLater() operations for that token+id will permanently fail during the success check within SeaportBasicRoute.sol:

// Receiving ERC1155
if (
    IERC1155(basicOrderParams.offerToken).balanceOf(address(this), basicOrderParams.offerIdentifier) !=
    basicOrderParams.offerAmount
) {
    revert OR_CallFailed(returnData);
}

Remediations to Consider

Consider overriding ERC1155Holder methods within PayLaterSeaportBasic to only accept tokens during a call to payLater().

Alternatively, consider updating the above _checkSuccess() logic to revert if the balanceOf() is < the offer amount, rather than strictly equal.

The logic in ItemsVerifier.sol and PunksVerifier.sol allows a malicious borrower to request a loan against a Vault with less collateral than indicated within the SignatureItems predicate. For example, a Vault may only contain a single BAYC token, but a malicious borrower can falsely indicate two (or many more) wildcard BAYC items as collateral, and the loan will successfully originate upon signing. Doing this, a malicious borrower can secure an under-collateralized loan, and intentionally default to achieve an immediate profit - at the expense of the actual collateral.

This occurs because verifyPredicates() within ItemsVerifier.sol and PunksVerifier.sol only checks that the Vault contains at least one asset token, without accounting for the possibility of multiple wildcard predicate items for the same asset. From ItemsVerifier.sol:

for (uint256 i = 0; i < items.length; i++) {
    SignatureItem memory item = items[i];

    ...

    if (item.cType == CollateralType.ERC_721) {
        ...

        // Wildcard, but vault has no assets
        if (id < 0 && asset.balanceOf(vault) == 0) return false;

Remediations to Consider

Potential options may include:

• Restrict to only a single wildcard per asset.
• Support amt values > 1 in the case of wildcards, and confirm asset balance accordingly.

Additionally, consider restricting predicate items - for a single asset - to have either a single wildcard or any number of specific ids. This avoids complicated scenarios where both specific id(s) and a wildcard are indicated for the same asset.

In FlashConsumerAAVE.sol, LENDING_POOL is initialized in constructor.

constructor(ILendingPoolAddressesProvider _addressesProvider) {
    ADDRESSES_PROVIDER = _addressesProvider;
    LENDING_POOL = ILendingPool(_addressesProvider.getLendingPool());
}

However, according to Aave’s documentation, this address can change and thus the correct LendingPool address should always be fetched dynamically

Remediations to Consider

Whenever LendingPool is used, consider fetching it dynamically via _addressesProvider.getLendingPool().

The feeController address can be changed via LoanCore::setFeeController(), but PayLater contracts will continue to utilize the old FeeController. If the origination fee is different between the versions, this may result in the PayLater operation either failing or locking excess funds in the PayLater contract, depending on whether the new origination fee has been raised or lowered.

Failures can be remedied by re-deploying PayLater contracts with updated feeController; and pointing UI to new contracts.

Remediations to Consider

Consider updating Immutables.sol to retrieve feeController at time of request, similar to this logic.

LoanCore.sol exclusively uses _msgSender(); OriginationController.sol and RepaymentController.sol vary in their uses of msg.sender  vs _msgSender(); contracts in the market repo largely use msg.sender. Consider normalizing all implementations to use only one of these.

• In FlashConsumerAAVE.sol and FlashConsumerBalancer.sol. This asset is the ERC20 offer token / payable currency being flash-loaned.
• Line 191, InstallmentsCalc.sol mentions that there shouldn’t be loans with more than 1 million installment periods. However, the actual limit defined in OriginationController::_validateLoanTerms is 1000.
• Line 125, AssetVault.sol mentions ERC20 token instead of ERC721

Consider updating those accordingly.

ERC721PermitUpgradeable.sol is missing a storage gap: adding a new storage variable may change the storage layout of VaultFactory upon upgrade, rendering it unusable. This is presently safeguarded from occurring by use of the preset OZ upgrade library.

Should there be a need to add state to ERC721PermitUpgradeable, consider doing so via unstructured storage.

RepaymentController.sol::repayPart is not access-controlled and so can be called by anyone and not only by the borrower itself. However, any overpaid amount is always refunded to the borrower, instead of the original sender.

See LoanCore::repayPart:

if (_paymentToPrincipal > _balanceToPay) {
    // Borrower overpaid, so send refund
    IERC20Upgradeable(data.terms.payableCurrency).safeTransfer(
        borrower,    
        _paymentToPrincipal - _balanceToPay
    );
}

Remediations to Consider

• Consider refunding the overpaid amount to the original sender instead of the borrower, or
• making the function accessible to the borrower only

The user documentation under step 6 of “Accept and Offer - Vault & NFT” wrongly mentions ERC271 instead of ERC721.

The rules of installments loans such as

• Lender can claim collateral after 40% payments have been missed, or
• “late fee” is charged for delayed payments

are well documented in code, but there is no higher-level documentation. Consider adding appropriate documentation to the user guide: https://docs.arcade.xyz/docs

In CollateralSaleSeaportBasic.sol::fulfillCollateralSale(), only the vault items specified are considered to be withdrawn. Thus any items that are hold by the vault but left out in the vaultItems argument, won’t be withdrawn to the original vault owner. As the CollateralSaleSeaportBasic contract will be the owner of the vault, all leftover items are locked up in the contract and can’t be re-claimed by the original owner. It requires an admin to call rescueAsset function on behalf of the user to withdraw assets back to original owner.

Owner-controlled functions should - if at all - only be used for admin operations but shouldn’t be used for business logic operations.

Remediations to Consider

Consider a solution which doesn't require any manual intervention by the owner. For example, adding whitelist capability to VaultFactory so that withdrawEnabled-vaults can be transferred only by certain whitelisted addresses (like CollateralSale).

1. draft-EIP712

   In ERC721Permit.sol, ItemsVerifier.sol, PunksVerifier.sol, and VaultInventoryReporter.sol, a deprecated version of EIP712 is imported:

   import "@openzeppelin/contracts/utils/cryptography/draft-EIP712.sol";
   

   Consider using the final version instead.

2. draft-EIP712Upgradable **In ERC721PermitUpgradeable.sol and OriginationController.sol, a deprecated version of EIP712Upgradable is imported:

   import "@openzeppelin/contracts-upgradeable/utils/cryptography/draft-EIP712Upgradeable.sol";
   

   Consider using the final version instead.

• The following import in RepaymentController.sol is not required and can be removed:

  import "@openzeppelin/contracts/utils/introspection/IERC165.sol";
  

• The following import in CollateralSaleSeaportBasic.sol and PlayLaterSeaportBasic.sol is not required and can be removed:

  *import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";*
  

In addition using SafeERC20 for IERC20; statement can be removed.

Any changes to RepaymentController will require deploy to a new address, and updating the REPAYER_ROLE within LoanCore to complete the ‘upgrade’. Subsequent calls to repayment functions within CollateralSale contracts will fail, due to the old repaymentController no longer having the REPAYER_ROLE.

The following optimizations can be made to save gas costs on for loops:

• use ++i instead of i++
• use unchecked increment

Before:

for (uint256 i; i < length); i++) { 
    ...
}

After:

for (uint256 i; i < length;) { 
    ...
        unchecked { ++i; }
}

Consider updating for loops accordingly:

• InstallmentsCalc.sol#L121
• LoanCore.sol#L499
• OriginationController#L229, OriginationController#L414, OriginationController#L614
• All for loops in VaultInventoryReporter.sol
• ItemsVerifier.sol#L88, PunksVerifier.sol#L60
• CollateralSale.sol#173
• PayLater.sol#112
• SeaportBasicRoute#86

Both loanCore and lenderNote are only set in constructor. Consider using the immutable keyword to save gas costs.

CollateralSaleSeaportBasic.sol#L161 defines local variable amountOwed:

uint256 amountOwed = amount + premium;

In subsequent _finishCallback call, amount + premium is passed as 2nd argument. Consider using pre-calculated amountOwed variable instead.

CollateralSaleSeaportBasic.sol#L77 calls LoanCore to retrieve loan terms:

LoanLibrary.LoanTerms memory terms = ILoanCore(loanCore).getLoan(loanId).terms;

Subsequently, _verifyLoanData is called where loan terms are fetched from LoanCore again. Consider avoiding additional external call to LoanCore by passing loan terms to _verifyLoanData function.