AssetVault Code Style Findings

AVT-01C: Loop Iterator Optimization

Type	Severity	Location
Gas Optimization		AssetVault.sol:L206

Description:

The linked for loop increments / decrements the iterator "safely" due to Solidity's built-in safe arithmetics (post-0.8.X).

Example:

Copy
206for (uint256 i = 0; i < tokensLength; i++) {

Recommendation:

We advise the increment / decrement operation to be performed in an unchecked code block as the last statement within the for loop to optimize its execution cost.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The loop iterator has been optimized as advised, relocating it to a dedicated unchecked code block that optimizes its execution.

AVT-02C: Misconception of Re-Entrancy Attack Vectors

Type	Severity	Location
Gas Optimization		AssetVault.sol:L224, L231

Description:

The AssetVault::withdrawETH function specifies that a potential re-entrancy attack vector can be introduced by the transmission of funds to the to address and that is why the ReentrancyGuard::nonReentrant modifier has been introduced to the function, however, the ReentrancyGuard::nonReentrant modifier will solely protect the functions it is specified in.

As the AssetVault::onlyWithdrawEnabled modifier is solely applied in this function, a potential re-entrancy would not protect the contract as it solely guards the AssetVault::withdrawETH function that will transmit all funds to the to address.

Example:

Copy
224function withdrawETH(address to) external override onlyOwner onlyWithdrawEnabled nonReentrant {
225    if (to == address(0)) revert AV_ZeroAddress("to");
226
227    // perform transfer
228    uint256 balance = address(this).balance;
229    // sendValue() internally uses call() which passes along all of
230    // the remaining gas, potentially introducing an attack vector
231    payable(to).sendValue(balance);
232    emit WithdrawETH(msg.sender, to, balance);
233}

Recommendation:

We advise the code to simply omit the ReentrancyGuard::nonReentrant modifier as it is practically ineffectual at the point it has been introduced.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The Arcade XYZ team evaluated this exhibit and opted to retain the current behaviour of the codebase.

AVT-03C: Optimization of if-else Structure

Type	Severity	Location
Gas Optimization		AssetVault.sol:L211

Description:

The referenced else-if clause is redundant as it will always evaluate to true based on the preceding clauses.

Example:

Copy
209if (tokenTypes[i] == TokenType.ERC721) {
210    _withdrawERC721(tokens[i], tokenIds[i], to);
211} else if (tokenTypes[i] == TokenType.ERC1155) {
212    _withdrawERC1155(tokens[i], tokenIds[i], to);
213}

Recommendation:

We advise it to be omitted, replacing it with a direct else clause thus optimizing the code's gas cost.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The if-else structure has been optimized as advised, reducing its gas cost on each iteration of the for loop its located in.

AVT-04C: Potential Enhancement of Functionality

Type	Severity	Location
Code Style		AssetVault.sol:L196, L209-L213

Description:

The AssetVault::withdrawBatch function does not presently support EIP-20 assets when it could, simplifying the extraction process of a vault via a single function.

Example:

Copy
195function withdrawBatch(
196    address[] calldata tokens,
197    uint256[] calldata tokenIds,
198    TokenType[] calldata tokenTypes,
199    address to
200) external override onlyOwner onlyWithdrawEnabled {
201    uint256 tokensLength = tokens.length;
202    if (tokensLength > MAX_WITHDRAW_ITEMS) revert AV_TooManyItems(tokensLength);
203    if (tokensLength != tokenIds.length) revert AV_LengthMismatch("tokenId");
204    if (tokensLength != tokenTypes.length) revert AV_LengthMismatch("tokenType");
205
206    for (uint256 i = 0; i < tokensLength; i++) {
207        if (tokens[i] == address(0)) revert AV_ZeroAddress("token");
208
209        if (tokenTypes[i] == TokenType.ERC721) {
210            _withdrawERC721(tokens[i], tokenIds[i], to);
211        } else if (tokenTypes[i] == TokenType.ERC1155) {
212            _withdrawERC1155(tokens[i], tokenIds[i], to);
213        }
214    }
215}

Recommendation:

We advise the AssetVault::withdrawBatch to be updated to support a token type of ERC20 that should also be introduced to the relevant TokenType enum.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The Arcade XYZ team evaluated this exhibit and opted to retain the current behaviour of the codebase as most use cases of the vault will involve EIP-721 and EIP-1155 assets.

As such, we consider this exhibit safely acknowledged.

AVT-05C: Potential Optimization of Project Structure

Type	Severity	Location
Gas Optimization		AssetVault.sol:L79-L82, L93-L100

Description:

The AssetVault contract is meant to be deployed once as a template that the VaultFactory will utilize.

Example:

Copy
69address public override whitelist;

Recommendation:

We advise template flow to be revised by having the VaultFactory deploy an instance of AssetVault during its VaultFactory::constructor and assign it in its template data entry. This will permit the ownershipToken of OwnableERC721 as well as the whitelist member of AssetVault to be set as immutable, greatly optimizing the gas cost of the contracts across the board.

Any immutable variable is compatible with proxy systems given that their values are "baked in" the bytecode of the deployed instance. We advise this and the relevant exhibit of OwnableERC721 to be applied in tandem if the optimizations are ultimately desired.

Alleviation (7a4e1dc948e94ded7385dbb74818bcf93ecc207c):

The Arcade XYZ team evaluated this exhibit and assessed that the flexibility of the VaultFactory will be greatly diminishing if the immutable based optimization is applied, a trait that the Arcade XYZ team does not wish to forfeit.

As such, we consider this exhibit safely acknowledged.