NFTXStakingZap, NFTXMarketplaceZap and NFTXVaultUpgradeable use hard coded Cryptokitties and CryptoPunks addresses

[code423n4]

Handle

hyh

Vulnerability details

Impact

With further system development it will be harder to track fixes needed for address upgrade. Hard coding induced errors are more probable and severe when the approach is used in multiple places in a big enough system.

Proof of Concept

NFTXStakingZap: transferFromERC721 https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXStakingZap.sol#L410 approveERC721 https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXStakingZap.sol#L434

NFTXMarketplaceZap: transferFromERC721 https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXMarketplaceZap.sol#L550 approveERC721 https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXMarketplaceZap.sol#L574

NFTXVaultUpgradeable: transferERC721 https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXVaultUpgradeable.sol#L486 transferFromERC721 https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXVaultUpgradeable.sol#L504

Recommended Mitigation Steps

Save the address to the storage variable. Add NFT groups formed according to the approach used to access them, i.e. generalize the access mechanics. Say the first group is accessed with transferFrom(address,address,uint256), put Cryptokitties, NFT1, NFT2 there, and so forth.

As this is helper constructions needed for access ease it is possible to allow changing the groups. It's also better to do it in a helper contract that is inherited by all the contracts above, so the NFT address access lists to exist in one place for transparency and manageability ease.

[0xKiwi]

We can reliably expect these hard coding wont cause a problem, but this is a fair report. Will look into deduplicating some important storage.