NFTX contest - shw's results

Handle

shw

Vulnerability details

Impact

In the contract NFTXFeeDistributor, the SafeMath library is not used when performing arithmetic operations, which could potentially integer overflow/underflow and cause unexpected results.

Proof of Concept

Referenced code: NFTXFeeDistributor.sol#L64-L65 NFTXFeeDistributor.sol#L91-L93 NFTXFeeDistributor.sol#L108 NFTXFeeDistributor.sol#L147 NFTXFeeDistributor.sol#L154

Tools Used

None

Recommended Mitigation Steps

Use SafeMath to prevent integer overflow/underflow.

Handle

shw

Vulnerability details

Impact

The variable eligibilityManager in the contract NFTXVaultFactoryUpgradeable is not set or used. However, this variable is referenced in the function deployEligibilityStorage of NFTXVaultUpgradeable, which should be initialized.

Proof of Concept

Referenced code: NFTXVaultFactoryUpgradeable.sol#L24 NFTXVaultUpgradeable.sol#L163-L177

Tools Used

None

Recommended Mitigation Steps

Add functions in NFTXVaultFactoryUpgradeable to set the variable eligibilityManager.

Handle

shw

Vulnerability details

Impact

The contract NFTXDeferEligibility includes a constructor; however, according to the documentation from Openzeppelin, it shouldn't:

Due to a requirement of the proxy-based upgradeability system, no constructors can be used in upgradeable contracts.

If we deploy the NFTXDeferEligibility through the upgrades.deployProxy function (as written in the tests), we will get an error:

contracts/solidity/eligibility/NFTXDeferEligibility.sol:23: Contract `NFTXDeferEligibility` has a constructor
    Define an initializer instead

Proof of Concept

Referenced code: NFTXDeferEligibility.sol#L23-L25

Tools Used

None

Recommended Mitigation Steps

Remove the constructor in the contract.

Handle

shw

Vulnerability details

Impact

During initialization, the contract NFTXDeferEligibility does not check whether the provided first parameter (i.e., _deferAddress) is non-zero. Besides, the contract NFTXUniqueEligibility does not check that the provided first and second parameters are non-zero (_owner, _vault) either. If any of these parameters are provided as zero accidentally, there is no chance to change the corresponding state variables, and the contracts should be redeployed.

Proof of Concept

Referenced code: NFTXDeferEligibility.sol#L44

Tools Used

None

Recommended Mitigation Steps

Add non-zero address checks, e.g., require(_vault != address(0)).

Handle

shw

Vulnerability details

Impact

In the NFTXMintRequestEligibility contract, the approveMintRequests and claimUnminted functions contain out-of-bound index access. Transactions calling these two functions will fail.

Proof of Concept

The dynamic array is not initialized with a given length and will be 0-length by default. Therefore, assigning the 0th element (at line 159, 160, 182, 183) of the array is considered out-of-bound.

Referenced code: NFTXMintRequestEligibility.sol#L157-L160 NFTXMintRequestEligibility.sol#L180-L183

Tools Used

None

Recommended Mitigation Steps

In both functions, change uint256[] memory _tokenIds; to uint256[] memory _tokenIds = new uint256[](1);, and uint256[] memory _amounts; to uint256[] memory _amounts = new uint256[](1);.

Handle

shw

Vulnerability details

Impact

In the contract ERC1155HolderUpgradeable, the function supportsInterface is not fully implemented, leading to errors for other contracts/Dapps that call this function. See the ERC165 specification for details.

Proof of Concept

Referenced code: ERC1155HolderUpgradeable.sol#L29-L34

Tools Used

None

Recommended Mitigation Steps

Implement the function by using a mapping to store the supported interfaces, as shown in the ERC165Storage.sol implementation from OpenZeppelin.

Handle

shw

Vulnerability details

Impact

In the contract NFTXLPStaking, the return values of transfer and transferFrom are sometimes checked to be true and sometimes not checked.

Proof of Concept

Referenced code: NFTXLPStaking.sol#L107 NFTXLPStaking.sol#L118 NFTXLPStaking.sol#L188

The following three lines in the code involve transfers of the rewardToken or stakingToken, but only one of them (at line 118) checks the result with a require statement. If stakingToken is not ERC20-compliant, then line 188 needs a require statement. Or, if stakingToken is sure to be ERC20-compliant, the require at line 118 is not necessary.

IERC20Upgradeable(pool.rewardToken).transferFrom(msg.sender, address(rewardDistToken), amount); // line 107
require(IERC20Upgradeable(pool.stakingToken).transferFrom(msg.sender, address(this), amount));  // line 118
IERC20Upgradeable(pool.stakingToken).transfer(account, amount); // line 188

Tools Used

None

Recommended Mitigation Steps

A more general and safer way is to use the safeTransfer or safeTransferFrom of the SafeERC20 implementation from OpenZeppelin.

Handle

shw

Vulnerability details

Impact

During initialization, if the first parameter uniLikeExchange is provided as 0 accidentally, there is no chance to update the corresponding state variable, and the contract should be redeployed.

Proof of Concept

Referenced code: StakingTokenProvider.sol#L25

Tools Used

None

Recommended Mitigation Steps

Add a non-zero address check, i.e., require(uniLikeExchange != address(0)).

Handle

shw

Vulnerability details

Impact

In the contract NFTXMintRequestEligibility, the state variable manager is not used. The state variable reverseEligOnRedeem is assigned to a value during initialization but not used afterwards. There are also unused events, CanApproveMintRequestsSet, Reject, and Approve.

Proof of Concept

Referenced code: NFTXMintRequestEligibility.sol#L28 NFTXMintRequestEligibility.sol#L31 NFTXMintRequestEligibility.sol#L44 NFTXMintRequestEligibility.sol#L50-L51

Tools Used

None

Recommended Mitigation Steps

Consider removing these variables and events to save gas.

Handle

shw

Vulnerability details

Impact

The function __NFTXEligibility_init_bytes in the contract NFTXEligibility can be declared external to save gas. Contracts that inherit NFTXEligibility and override __NFTXEligibility_init_bytes, i.e., the NFTX*Eligibility ones, can also be declared external. Please refer to the following links as details.

Proof of Concept

Referenced code: NFTXEligibility.sol#L13 NFTXDeferEligibility.sol#L32-L37 NFTXListEligibility.sol#L28-L33 NFTXMintRequestEligibility.sol#L58-L68 NFTXRangeEligibility.sol#L44-L54 (NFTXUniqueEligibility.sol#L47-L57)[https://github.com/code-423n4/2021-05-nftx/blob/main/nftx-protocol-v2/contracts/solidity/eligibility/NFTXUniqueEligibility.sol#L47-L57]

Please refer to the files 500.js, 500-original.png, and 500-changed.png in the following link. PoC: Link to PoC

Tools Used

None

Recommended Mitigation Steps

Change public to external and bytes memory to bytes calldata.