VaultRegistry Manual Review Findings

VRY-01M: Contradictory Documentation of Statements

Type	Severity	Location
Logical Fault		VaultRegistry.sol: • I-1: L200-L201 • I-2: L286-L287

Description:

The referenced statements ensure that a strategy for the specified token ID exists yet each statement's documentation implies it validates the opposite.

Example:

Copy
200//Validate that no strategy exists of the tokenid passed
201strategyRegistry.ownerOf(_tokenId);

Recommendation:

We advise either the documentation or the code to be revised, either of which we consider acceptable.

Alleviation (fbb15dfb5ea6fad8296e934a8eb87c9fc3ef13cd):

The documentation of the system was properly updated to reflect that existence of the relevant strategies are meant to be enforced, addressing this exhibit.

VRY-02M: Non-Standard Disable of Initializer

Type	Severity	Location
Standard Conformity		VaultRegistry.sol:L150

Description:

The VaultRegistry::constructor will disable its Initializable implementation by invoking the Initializable::initializer modifier in its declaration rather than invoking the Initializable::_disableInitializers function.

Impact:

If the VaultRegistry were to ever use versioned initializations, they would still be invoke-able in the base implementation due to an incorrect initialization mechanism in the contract's constructor.

Example:

Copy
150constructor() initializer {}

Recommendation:

We advise the relevant disable function to be invoked, ensuring that versioned initializations are also prohibited.

Alleviation (fbb15dfb5ea6fad8296e934a8eb87c9fc3ef13cd):

The Steer Protocol team evaluated this exhibit and clarified that the particular version of Initializable they import to the codebase does not support the Initializable::_disableInitializers flow.

After confirming this is indeed the case, we consider this exhibit to be inapplicable.

VRY-03M: Denial-of-Service of Vault Creation

Type	Severity	Location
Logical Fault		VaultRegistry.sol:L311

Description:

The VaultRegistry::createUniswapHookVault function will utilize a user-provided _salt argument that is not influenced by contextual variables, permitting a user to deploy a vault with a different IPFS hash and token ID on the same newHook address and thus cause the code to misbehave.

Impact:

Depending on the sensitivity of the token ID and IPFS variables, a vault's creation can be forced to fail and a vault that is misconfigured can be deployed in its place.

Example:

Copy
311bytes32 saltBytes = bytes32(_salt);
312
313assembly {
314    newHook := create2(
315        0,
316        add(creationCode, 0x20),
317        mload(creationCode),
318        saltBytes
319    )
320}

Recommendation:

We advise the code to combine the user's provided _salt with their address (msg.sender), ensuring create2 deployments are unique per caller.

Alleviation (fbb15dfb5ea6fad8296e934a8eb87c9fc3ef13cd):

The code was updated to utilize a salt value bound to the caller, preventing Denial-of-Service attacks from manifesting.

VRY-04M: Inexistent Handling of Creation Failure

Type	Severity	Location
Logical Fault		VaultRegistry.sol:L313-L320

Description:

The create2 opcode in the assembly block can yield the zero-address if it fails (f.e. by re-using the same _salt), however, the code continues normally and thus creates corrupt data entries.

Impact:

A Uniswap hook vault failure will remain undetected and will mutate the storage entries of the VaultRegistry incorrectly.

Example:

Copy
313assembly {
314    newHook := create2(
315        0,
316        add(creationCode, 0x20),
317        mload(creationCode),
318        saltBytes
319    )
320}
321
322// Add beacon type to mapping
323beaconTypes[address(newHook)] = _beaconName;
324// As the vaultypes will be gradually upgarded one by one there may be situations where
325// there may be a need to create older type of vaults(as they are not yet upgraded) which
326// do not depending on feemanager and also
327// there may be a need to craete the newer vaults which are depending on the fee manager so
328// to maintain backward compatibilty below piece of code is put in try catch block
329// Also in future there may be a vault type that does not use fee manager
330try IMultiPositionManager(address(newHook)).feeDetails() returns (
331    uint256 totalFees,
332    address[] memory feeWithdrawers,
333    string[] memory feeIdentifiers,
334    uint256[] memory feeValues
335) {
336    if (totalFees > 0) {
337        IFeeManager(feeManager).setDefaultFeeAndWithdrawalPermission(
338            address(newHook),
339            totalFees,
340            feeIdentifiers,
341            feeValues,
342            feeWithdrawers
343        );
344    }
345} catch {
346    emit FeeSettingFailed(
347        address(newHook),
348        "Fee setting failed in FeeManager"
349    );
350}
351
352// Add enumeration for the vault
353_addLinkedVaultsEnumeration(
354    _tokenId,
355    address(newHook),
356    _payloadIpfs,
357    _beaconName
358);
359
360// Emit vault details
361emit VaultCreated(
362    msg.sender,
363    address(newHook),
364    _beaconName,
365    _tokenId,
366    _vaultManager
367);

Recommendation:

We advise the code to prevent such a case by ensuring that the newHook is non-zero after the assembly block.

Alleviation (fbb15dfb5ea6fad8296e934a8eb87c9fc3ef13cd):

The code was updated to ensure the new hook has been created properly, alleviating this exhibit.