CallBlacklist Manual Review Findings

CBT-01M: Inexistent Flexibility of Blacklist

Type	Severity	Location
Logical Fault		CallBlacklist.sol:L56-L69

Description:

The referenced restriction list imposed by CallBlacklist::isBlacklisted is immutable and is applied via a long OR (||) conditional chain.

Impact:

While this exhibit relates to potential enhancement of functionality, we consider it imperative given that we have showcased a potential function signature that could lead to the compromise of the AssetVault instance's funds.

Example:

Copy
55function isBlacklisted(bytes4 selector) public pure returns (bool) {
56    return
57        selector == ERC20_TRANSFER ||
58        selector == ERC20_ERC721_APPROVE ||
59        selector == ERC20_ERC721_TRANSFER_FROM ||
60        selector == ERC20_INCREASE_ALLOWANCE ||
61        selector == ERC721_SAFE_TRANSFER_FROM ||
62        selector == ERC721_SAFE_TRANSFER_FROM_DATA ||
63        selector == ERC721_ERC1155_SET_APPROVAL ||
64        selector == ERC1155_SAFE_TRANSFER_FROM ||
65        selector == ERC1155_SAFE_BATCH_TRANSFER_FROM ||
66        selector == PUNKS_TRANSFER ||
67        selector == PUNKS_OFFER ||
68        selector == PUNKS_OFFER_TO_ADDRESS ||
69        selector == PUNKS_BUY;
70}

Recommendation:

We advise the code to utilize a mutable mapping entry instead, permitting the owners of the blacklist to update it to accommodate for future collateral types that an AssetVault is expected to support.

As a security measure, the mapping entry could be solely written to for blacklisting and to not possess any un-blacklisting functionality as an AssetVault can always be marked withdrawable.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The Arcade XYZ team stated that they do not wish to add an administrative capacity to the CallBlacklist contract and that it is expected to be a static and immutable list of calls that are disallowed. As such, we consider this exhibit acknowledged.

CBT-02M: Potentially Weak Restriction List

Type	Severity	Location
Language Specific		CallBlacklist.sol:L28-L31, L33-L35, L37-L38, L40-L43

Description:

The function signature restriction list defined in CallBlacklist appears to lack functions that could still impact assets held by the contract, namely burn operations.

Impact:

As the CallBlacklist is a crucial component that is meant to prohibit the compromise of an AssetVault instance's collateral, we consider this exhibit to be of "major" severity. It is currently possible to "sabotage" a potential loan by burning the assets held within it after it has been created, harming a lender's position.

Example:

Copy
25/**
26 * @dev Global blacklist for transfer functions.
27 */
28bytes4 private constant ERC20_TRANSFER = IERC20.transfer.selector;
29bytes4 private constant ERC20_ERC721_APPROVE = IERC20.approve.selector;
30bytes4 private constant ERC20_ERC721_TRANSFER_FROM = IERC20.transferFrom.selector;
31bytes4 private constant ERC20_INCREASE_ALLOWANCE = bytes4(keccak256("increaseAllowance(address,uint256)"));
32
33bytes4 private constant ERC721_SAFE_TRANSFER_FROM = bytes4(keccak256("safeTransferFrom(address,address,uint256)"));
34bytes4 private constant ERC721_SAFE_TRANSFER_FROM_DATA = bytes4(keccak256("safeTransferFrom(address,address,uint256,bytes)"));
35bytes4 private constant ERC721_ERC1155_SET_APPROVAL = IERC721.setApprovalForAll.selector;
36
37bytes4 private constant ERC1155_SAFE_TRANSFER_FROM = IERC1155.safeTransferFrom.selector;
38bytes4 private constant ERC1155_SAFE_BATCH_TRANSFER_FROM = IERC1155.safeBatchTransferFrom.selector;
39
40bytes4 private constant PUNKS_TRANSFER = bytes4(keccak256("transferPunk(address,uint256)"));
41bytes4 private constant PUNKS_OFFER = bytes4(keccak256("offerPunkForSale(uint256,uint256)"));
42bytes4 private constant PUNKS_OFFER_TO_ADDRESS = bytes4(keccak256("offerPunkForSaleToAddress(uint256,uint256,address)"));
43bytes4 private constant PUNKS_BUY = bytes4(keccak256("buyPunk(uint256)"));

Recommendation:

Given that burn functionality is permitted in multiple token implementations, we advise the IERC20::burn (and its EIP-721 counterpart) to be blocked as well as the IERC20::burnFrom function.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

Multiple function signatures relating to burn functionality have been introduced and are now disallowed by the CallBlacklist::isBlacklisted function, alleviating this exhibit.

As an addendum, the Arcade XYZ team stated that the CallBlacklist is not meant to be a comprehensive blacklist and that extra care should be taken by users when whitelisting functions as they may still affect vault assets.