Security Audit
February 6th, 2023
Version 1.0.0
Presented by 0xMacro
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.
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.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
96933d4ad1f4c2e58227f4e86faa6c6e013514f6
298694677783ac6e16cdfbcf4fa4e5ea47e1a350
We audited the following contracts within the v2-contracts repository:
Contract | SHA256 |
---|---|
contracts/ERC721Permit.sol |
|
contracts/ERC721PermitUpgradeable.sol |
|
contracts/InstallmentsCalc.sol |
|
contracts/LoanCore.sol |
|
contracts/OriginationController.sol |
|
contracts/PromissoryNote.sol |
|
contracts/RepaymentController.sol |
|
contracts/vault/AssetVault.sol |
|
contracts/vault/CallWhitelist.sol |
|
contracts/vault/CallWhitelistApprovals.sol |
|
contracts/vault/OwnableERC721.sol |
|
contracts/vault/VaultDepositRouter.sol |
|
contracts/vault/VaultFactory.sol |
|
contracts/vault/VaultInventoryReporter.sol |
|
contracts/vault/VaultOwnershipChecker.sol |
|
contracts/verifiers/ItemsVerifier.sol |
|
contracts/verifiers/PunksVerifier.sol |
|
contracts/libraries/LoanLibrary.sol |
|
We audited the following contracts within the market repository:
Contract | SHA256 |
---|---|
contracts/lending-plus/base/CollateralSale.sol |
|
contracts/lending-plus/base/Immutables.sol |
|
contracts/lending-plus/base/PayLater.sol |
|
contracts/lending-plus/final/CollateralSaleFinal.sol |
|
contracts/lending-plus/final/PayLaterFinal.sol |
|
contracts/lending-plus/order-routing/SeaportBasicRoute.sol |
|
contracts/lending-plus/order-routing/TreasureRoute.sol |
|
contracts/lending-plus/seaport-basic/CollateralSaleSeaportBasic.sol |
|
contracts/lending-plus/seaport-basic/PayLaterSeaportBasic.sol |
|
contracts/flash/FlashConsumerAAVE.sol |
|
contracts/flash/FlashConsumerBalancer.sol |
|
contracts/flash/FlashConsumerBase.sol |
|
contracts/libraries/LendingPlusLibrary.sol |
|
contracts/libraries/LoanLibrary.sol |
|
contracts/interfaces/ICollateralSale.sol |
|
contracts/interfaces/IPayLater.sol |
|
Click on an issue to jump to it, or scroll down to see them all.
PayLater
can be permanently blocked for a specific ERC1155 token+id
feeController
may cause unexpected behavior in PayLater contracts
_msgSender()
for ERC2771 support
ERC721PermitUpgradeable.sol
RepaymentController
require re-deploy of CollateralSale contracts
immutable
for state variables in RepaymentController
amountOwed
LoanCore
We quantify issues in three parts:
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. |
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:
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.
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:
repayPart
with minimum payment of 1 etherrepayPart
10 times (as number of installments are 10) with amount of only 1 weiNote 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
minBalDue
as calculated in InstallmentsCalc::_calcAmountsDue
numInstallmentsPaid
in LoanCore.repayPart
based on current loan balanceERC777 - 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:
ERC777Token
interface implementer. Consider preventing the loan in this scenario.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: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.
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.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
PayLater
can be permanently blocked for a specific ERC1155 token+id
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:
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()
.
feeController
may cause unexpected behavior in PayLater contracts
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.
_msgSender()
for ERC2771 support
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.
asset
is the ERC20 offer token / payable currency being flash-loaned.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
.AssetVault.sol
mentions ERC20 token instead of ERC721Consider updating those accordingly.
ERC721PermitUpgradeable.sol
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
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
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
).
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.
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.
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 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.
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.
RepaymentController
require re-deploy of CollateralSale contracts
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 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.
The following optimizations can be made to save gas costs on for loops:
++i
instead of i++
unchecked
incrementBefore:
for (uint256 i; i < length); i++) {
...
}
After:
for (uint256 i; i < length;) {
...
unchecked { ++i; }
}
Consider updating for loops accordingly:
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.
immutable
for state variables in RepaymentController
Both loanCore
and lenderNote
are only set in constructor. Consider using the immutable
keyword to save gas costs.
amountOwed
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.
LoanCore
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.
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.