Handle

0xRajeev

Vulnerability details

Impact

The vault manager has all the privileged administrative control over a vault such as enabling it, setting different fees and deciding eligibility criteria of vault NFTs. The privileged actions can only be performed by the manager as long as it is non-zero i.e. not finalized. Once finalized, the contract owner can execute these privileged operations.

The setManager() function to update the manager address is a single-step process where the manager address is changed to the _manager parameter provided. This is dangerous if the supplied parameter is accidentally an incorrect address to which the manager does not have the private key.

In such a scenario, the impact is that manager address is still non-zero but cannot execute any of the privileged administration functions. The owner cannot change or override because the manager address is non-zero. The manager itself cannot rectify this mistake because it does not have the private key to the newly/incorrectly set manager address. This freezes vault administration and will require vault contract redeployment.

While enabling and setting eligibility is a one-time action, it may be desirable to call setFees() multiple times to change the fees over time which will not be possible in this scenario.

Proof of Concept

https://github.com/code-423n4/2021-05-nftx/blob/f6d793c136d110774de259d9f3b25d003c4f8098/nftx-protocol-v2/contracts/solidity/NFTXVaultUpgradeable.sol#L193-L198

https://github.com/code-423n4/2021-05-nftx/blob/f6d793c136d110774de259d9f3b25d003c4f8098/nftx-protocol-v2/contracts/solidity/NFTXVaultUpgradeable.sol#L429-L435

Tools Used

Manual Analysis

Recommended Mitigation Steps

Change the updating of manager address from a single-step setManager() function to two steps where the first step proposes new ownership in one transaction and a second transaction from the newly proposed manager address accepts ownership. A mistake in the first step can be recovered by granting with a new correct address again before the new manager address accepts ownership. Ideally, there should also be a timelock enforced before the new manager takes effect.

code-423n4 / 2021-05-nftx-findings