Reach out for an audit or to learn more about Macro
or Message on Telegram

Arcade A-2

Security Audit

February 6th, 2023

Version 1.0.0

Presented by 0xMacro

Table of Contents

Introduction

This document includes the results of the security audit for Arcade's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from December 19, 2022 to January 18, 2023.

The purpose of this audit is to review the source code of certain Arcade Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.

Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.

Overall Assessment

The following is an aggregation of issues found by the Macro Audit team:

Severity Count Acknowledged Won't Do Addressed
Critical 2 - - 2
High 3 - 1 2
Medium 5 - 1 4
Low 1 - - 1
Code Quality 9 4 1 4
Informational 1 1 - -
Gas Optimization 4 - 1 3

Arcade was quick to respond to these issues.

Specification

Our understanding of the specification was based on the following sources:

Source Code

The following source code was reviewed during the audit:

We audited the following contracts within the v2-contracts repository:

Source Code SHA256
contracts/ERC721Permit.sol

0654e248d42cbf2068b47c22a111c861ce091098553986089d8160e1efdf4d74

contracts/ERC721PermitUpgradeable.sol

c337c89272f9940d7623b028f400461be8dd89979e243e2f6ddbbc5fe7ef190b

contracts/InstallmentsCalc.sol

0ca3972c5932e3646bb9633f1e5a954f3cceca7ae19799d0e62d73bf63da64ae

contracts/LoanCore.sol

88ab978368bc0db7d7283ef9402ecfafd6555696aa333959ed6969b94053923f

contracts/OriginationController.sol

443d97e05d489c6ff5701b8a385472a2397792be0218cef21939b9b1498e65a0

contracts/PromissoryNote.sol

ac11f4080a3afd1d8be6da45e4db34762df7af94398b834bd9bfdf8e36e41a37

contracts/RepaymentController.sol

8d61873419ff361ed20e1292dc9ba499266d81d35e0dead469ea2789b700c78a

contracts/vault/AssetVault.sol

6f4f2bcc8b2ca2829d83d3b46a590d53697c2185ff33823c82157573749f5237

contracts/vault/CallWhitelist.sol

7d5ce3a49f120d14e68e4ea5319ea0a092309f7caa1583e203d26b2fe50d9811

contracts/vault/CallWhitelistApprovals.sol

83c924ad6e8e58351aa5597a9a2329f86c094104f70e39b08e2aad0a2145f2b4

contracts/vault/OwnableERC721.sol

3f8529466b73248e12f09c0052bed60e71f7b7a7155193ec6ad158935d40c158

contracts/vault/VaultDepositRouter.sol

99ab8b6db9f8528f15cad7f4dec35ac9301f6fea40e48a3c60538df983566a7c

contracts/vault/VaultFactory.sol

99e762f872bc7931eb2dda19f7735d2e76104e33017a47f6210ef8937d3ac40e

contracts/vault/VaultInventoryReporter.sol

b55d7e112308074fac57d1f1ab49b64d8293696fa16e3fcb16010b6bbb886241

contracts/vault/VaultOwnershipChecker.sol

5616339992964af324d51725cec47cb7fc13d4d81e7c130809b54eabe500955f

contracts/verifiers/ItemsVerifier.sol

a1baf6e61d8c66de769c3fe3101f8ed4c820391dd9955192ef63be66035fc39d

contracts/verifiers/PunksVerifier.sol

b3fd90257abd460e4dd6d2184eae61b1cd345c37e086fa28f632e469c5995dda

contracts/libraries/LoanLibrary.sol

64ce829a964d65335618240ab1df24e6d507aa94a0b746a16c2a0a5142233b20

We audited the following contracts within the market repository:

Source Code SHA256
contracts/lending-plus/base/CollateralSale.sol

a9d7b6a97b390b912d81e2a04540cec86ec113c1ec9f0ea2176e07284b11724b

contracts/lending-plus/base/Immutables.sol

24be13c436d56c591cf3394181824ab8a8ce88fb6d8c809e4f66dce6d3bac4ec

contracts/lending-plus/base/PayLater.sol

e3c2ebb4ce649587ae8ddb271c04394ae4b3c102896b86234328933fed496337

contracts/lending-plus/final/CollateralSaleFinal.sol

a2c664231ef8307d4d3a1a6ff5ac064516e32b9210a70f235492c0793035e63f

contracts/lending-plus/final/PayLaterFinal.sol

bc26fa6349a5893b6b50a98774a79335da4a20e9640c7733a05dd855d59810b0

contracts/lending-plus/order-routing/SeaportBasicRoute.sol

2f6d91744f6965a562a1226dee061330ece6b1f233ba7d5b2c52bf5113752591

contracts/lending-plus/order-routing/TreasureRoute.sol

f9eb76008d18ef19c51eb8f04379564f324fcf1839f09077e55adad5c7fffca0

contracts/lending-plus/seaport-basic/CollateralSaleSeaportBasic.sol

37a8c8ba7584ffb996ce6881812892f609fbd80fe20b0b49dcea69c7d90a3a9f

contracts/lending-plus/seaport-basic/PayLaterSeaportBasic.sol

e8ab34cd83723a53885943d1fe0cdaa7e4f2b6f33d6ab382ff6723852f7d051a

contracts/flash/FlashConsumerAAVE.sol

4a847596337bcc7bd759254e9a65a3bf6324f117712cac6e79918dd40a13c567

contracts/flash/FlashConsumerBalancer.sol

54651e7eec2dd303cc149aac42c128409229049231dd208ebc8232b59920eb25

contracts/flash/FlashConsumerBase.sol

984a19712a91b10d318f43385cae230f48af905179e4407152d182e348d3d97e

contracts/libraries/LendingPlusLibrary.sol

df1f58cf5042673c8ba795c315256768d52b75a57a2ef2a85ec72f2ad3d26aba

contracts/libraries/LoanLibrary.sol

a6789ada33e81ab8709c9a18dd73c1b8380c846d0b8b09c27b192c46fc55bd46

contracts/interfaces/ICollateralSale.sol

bc0adf2b64defe9891c678304b485bb50a55c625e4d9a3eeaa03f684da0d04d3

contracts/interfaces/IPayLater.sol

1ded3abb4f65363dfc42e7f607bf724827284da905f20ea37ce347629271caaa

Issue Descriptions and Recommendations

Click on an issue to jump to it, or scroll down to see them all.

Security Level Reference

We quantify issues in three parts:

  1. The high/medium/low/spec-breaking impact of the issue:
    • How bad things can get (for a vulnerability)
    • The significance of an improvement (for a code quality issue)
    • The amount of gas saved (for a gas optimization)
  2. The high/medium/low likelihood of the issue:
    • How likely is the issue to occur (for a vulnerability)
  3. The overall critical/high/medium/low severity of the issue.

This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:

Severity Description
(C-x)
Critical

We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost.

(H-x)
High

We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec.

(M-x)
Medium

We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner.

(L-x)
Low

The risk is small, unlikely, or may not relevant to the project in a meaningful way.

Whether or not the project wants to develop a fix is up to the goals and needs of the project.

(Q-x)
Code Quality

The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design.

(I-x)
Informational

Warnings and things to keep in mind when operating the protocol. No immediate action required.

(G-x)
Gas Optimizations

The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it.

Issue Details

C-1

Seaport’s success check can permanently fail making the entire “collateral sale” functionality unusable

Topic
Griefing
Status
Impact
High
Likelihood
High

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);
}
C-2

“Collateral sale” doesn’t handle ERC1155 assets correctly

Topic
Griefing
Status
Impact
High
Likelihood
High

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.

H-1

Payable currency with transfer tax prevents borrowers from repaying, forcing loan defaults

Topic
Incentive Design
Status
Wont Do
Impact
High
Likelihood
Medium

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.
Response by Arcade

Nonstandard ERC20 token behavior has been previously reported in other audits and public bug bounty programs. We don’t plan to add special-casing for nonstandard tokens in response to this reported issue. In general, given the design of our protocol and the peer-to-peer nature of the mechanism, each counterparty must take care to be aware of the behaviors of the payable currency they are agreeing to, or the collateral they are lending against. Users should always be careful if they receive a loan offer they must originate outside of the Arcade UI, since any tax-on-transfer token will not be supported in our UI product. Therefore this is more of a phishing/social engineering vector as opposed to a core protocol issue, as we currently classify it.

In the future, we plan to eliminate this class of nonstandard behavior issues by implementing an on-chain whitelist for payable currency. At such time we can eliminate more pathways through which a malicious counterparty can use social engineering to trick their counterparty, including the pathway discussed in this report. Until such time as we do this, it is fully the users’ responsibility to ensure they know the terms of their agreement with their counterparty. As suggested in the [M-1] remediation, we’ll work on making this as clear as possible in our documentation.

H-2

ERC721 and ERC1155 tokens with id > max(int256) cannot be explicitly collateralized for loan origination

Topic
Spec
Status
Impact
Spec Breaking
Likelihood
Medium

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.

H-3

Lender only receives a small portion of interests for installment loans

Topic
Incentive Design
Status
Impact
Medium
Likelihood
High

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
M-1

An ERC777 payable currency allows malicious lenders to recoup nearly the full loan, then force a default to also take collateral

Topic
Incentive Design
Status
Wont Do
Impact
High
Likelihood
Low

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).
Response by Arcade

See response to [H-1] - this issue falls within the same category. In the future, on-chain whitelisting will preclude this vector, but in the meantime, counterparties must be aware and take caution that they are not creating this vector for themselves by agreeing to loans in such a currency.

M-2

Certain approve/transfer functions can be whitelisted, creating trust concerns for users.

Topic
Trust Model
Status
Impact
High
Likelihood
Low

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

M-3

PayLater can be permanently blocked for a specific ERC1155 token+id

Topic
Griefing
Status
Impact
Medium
Likelihood
Low

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.

M-4

Malicious borrower can secure an under-collateralized loan for immediate profit

Topic
Incentive Design
Status
Impact
High
Likelihood
Low

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.

M-5

Aave flash loan may reference outdated lending pool

Topic
3rd Party Behavior
Status
Impact
Medium
Likelihood
Medium

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().

L-1

Updates to feeController may cause unexpected behavior in PayLater contracts

Topic
Protocol Design
Status
Impact
Medium
Likelihood
Low

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.

Q-1

Inconsistent use of _msgSender() for ERC2771 support

Topic
Code Quality
Status
Quality Impact
Low

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.

Q-2

Inaccurate comments

Topic
Code Quality
Quality Impact
Low

Consider updating those accordingly.

Q-3

Missing storage gap in ERC721PermitUpgradeable.sol

Topic
Code Quality
Status
Acknowledged
Quality Impact
Low

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.

Q-4

Overpaid amount is refunded to borrower instead of original sender

Topic
Code Quality
Status
Quality Impact
Low

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
Q-5

Typo in documentation

Topic
Code Quality
Status
Acknowledged
Quality Impact
Low

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

Q-6

Lack of documentation on installment loans

Topic
Code Quality
Status
Acknowledged
Quality Impact
Low

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

Q-7

Locked vault items in “collateral sale” contract requires admin action to refund items back to original owner.

Topic
Code Quality
Status
Wont Do
Quality Impact
Low

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).

Response by Arcade

The Arcade team considered the implemented design against the suggested alternative, and reached the conclusion that no changes were necessary.

In general, rescueAsset should not be used in the course of normal CollateralSale business logic - if vaultItems are correctly specified, it never needs to be used. Instead, the rescue mechanism is only is meant to be a recovery scenario in case an item held in the vault was not specified by the user in the vaultItems array.

The Arcade team found it desiriable to maintain the optionality to recover from user error, but makes no guarantees about post-launch processes to do so, or even that we will do so. It’s possible that we won’t offer rescue ability to users who make routine mistakes, and will only do so in extreme or high-value scenarios.

We chose not to pursue the alternative of making withdrawEnabled vaults transferrable by specially-designated contracts, given that it breaks the logical and architectural independence of vaults from the core mechanisms of the protocol itself.

Q-8

Use of deprecated versions

Topic
Code Quality
Status
Acknowledged
Quality Impact
Low
  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.

Response by Arcade

The Arcade team understands the imported versions are deprecated, however replacing them with final versions led to dependency management issues. We elected to not spend effort on dependency management at this time, given that the draft versions also match what our deployed protocol uses on-chain. Before the next major protocol release, the Arcade team will revisit this issue.

Q-9

Unused imports

Topic
Code Quality
Quality Impact
Low
  • 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.

I-1

Updates to RepaymentController require re-deploy of CollateralSale contracts

Topic
Informational
Status
Acknowledged
Impact
Informational

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.

Response by Arcade

The Arcade team wishes to pursue the path of using upgradeability as a last resort, and ideally only in security-related scenarios. We believe that the increased gas costs of an additional downstream redeploy (of CollateralSale) outweigh the operational risks of performing an upgrade and possible introducing an error in new code or as a result of the upgrade process.

G-1

For loop optimization

Topic
Gas Optimization
Status
Wont Do
Gas Savings
Low

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:

Response by Arcade

Gas savings aside, the Arcade team finds the use of unchecked in for-loops to be an antipattern that increases contextual overhead and makes the learning curve steeper for new Solidity developers and first-time readers of the code. In our opinion, the unchecked optimization for for-loops is one that should be built into the compiler itself, and never present in Solidity code.

G-2

Use immutable for state variables in RepaymentController

Topic
Gas Optimization
Status
Gas Savings
Low

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

G-3

Use predefined local variable amountOwed

Topic
Gas Optimization
Status
Gas Savings
Low

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.

G-4

Avoid extra call to LoanCore

Topic
Gas Optimization
Status
Gas Savings
Low

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.

Disclaimer

Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.

Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.

The scope of this report and review is limited to a review of only the code presented by the Arcade team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.