Bundles/Airdrop - NFTfi

When transferring the ownership of the protocol, no checks are performed on whether the new address is valid and active. In case there is a mistake when transferring the ownership, the whole protocol may lose all of its ownership functionalities.

Code Location

Ownable.sol

    /**
     * @dev Transfers ownership of the contract to a new account (`newOwner`).
     * Can only be called by the current owner.
     */
    function transferOwnership(address _newOwner) public virtual onlyOwner {
        require(_newOwner != address(0), "Ownable: new owner is the zero address");
        _setOwner(_newOwner);
    }

SOLVED: The NFTfi team solved the issue by implementing a two-step ownership transfer process.

Commit ID: 52f68e41a729e83f27c1cb747a464a2367132d5b

Users can call sendElementsToPersonalBundler() function to move every token inside a bundle to a personal bundle. This function uses a while loop to iterate through every childToken of every childContract until childContracts[_tokenId] and childTokens[_tokenId][childContrac] lengths are 0, meaning that no more child tokens are held in the bundle.

However, if tokens are already in a personal bundle, and they are transferred to the same bundle, or if they are in a NftfiBundler bundle with id = 1 and they're being transferred to the same NftfiBundler bundle (the second scenario is less likely than the first one), the function runs into an infinite loop, since the lengths mentioned above will never decrease, keeping the while loop running until it spends the max amount of gas allowed for the call, reverting the state and incurring unnecessary cost to the user.

Code Location

NftfiBundler.sol

    /**
     * @notice Remove all the children from the bundle and send to personla bundler.
     * If bundle contains a legacy ERC721 element, this will not work.
     * @dev This method may run out of gas if the list of children is too big. In that case, children can be removed
     *      individually.
     * @param _tokenId the id of the bundle
     * @param _personalBundler address of the receiver of the children
     */
    function sendElementsToPersonalBundler(uint256 _tokenId, address _personalBundler) external {
        _validateReceiver(_personalBundler);
        _validateTransferSender(_tokenId);

        //fix this actual personalBundlerExists
        require(
            IERC165(_personalBundler).supportsInterface(type(IERC998ERC721TopDown).interfaceId),
            "has to implement IERC998ERC721TopDown"
        );
        uint256 personalBundleId = 1;
        //make sure sendeer owns personal bundler token
        require(IERC721(_personalBundler).ownerOf(personalBundleId) == msg.sender, "has to own personal bundle token");

        // In each iteration all contracts children are removed, so eventually all contracts are removed
        while (childContracts[_tokenId].length() > 0) {
            address childContract = childContracts[_tokenId].at(0);

            // In each iteration a child is removed, so eventually all contracts children are removed
            while (childTokens[_tokenId][childContract].length() > 0) {
                uint256 childId = childTokens[_tokenId][childContract].at(0);

                _removeChild(_tokenId, childContract, childId);

                try
                    IERC721(childContract).safeTransferFrom(
                        address(this),
                        _personalBundler,
                        childId,
                        abi.encodePacked(personalBundleId)
                    )
                {
                    // solhint-disable-previous-line no-empty-blocks
                } catch {
                    revert("only safe transfer");
                }
                emit TransferChild(_tokenId, _personalBundler, childContract, childId);
            }
        }
    }

As a proof of concept, user2 bundles NFTs 1, 2, and 3 with the NftfiBundler contract. From there, the NFTs are being transferred to the user2's personal bundler with the sendElementsToPersonalBundler() function, and then they are transferred again to the same personalbundle. This makes the sendElementsToPersonalBundler() function to run into an infinite loop, which ends up with crashing the test environment.

SOLVED: The NFTfi team solved the issue by preventing sendElementsToPersonalBundler() from being called with msg.sender as the _personalBundler address.

Commit ID: 478ae0542a50367defd1f39047f418806205f7aa

Users can use functions to add or remove multiple NFTs at the same time in the NftfiBundler or PersonalBundler contracts. These functions can have high gas costs based on the number of tokens transferred. Adding elements also calls an external validator contract to check whether the asset is permitted or not, further increasing the gas cost.

Many users use wallets with default gas limit configured. When the limit is reached, the users lose a significant amount of Ether in those failed transactions.

The affected functions:

NftfiBundler.sol

• buildBundle
• addBundleElements
• removeBundleElements
• addAndRemoveBundleElements
• decomposeBundle
• sendElementsToPersonalBundler

RISK ACCEPTED: The NFTfi team accepted the risk of this finding. In addition, gas limit and maximum bundle size checks will be implemented in the front-end.

The childContractByIndex and childTokenByIndex functions of the ERC998TopDown contract did not validate their parameters. Setting invalid values may result in reverts without error messages.

contracts/NftfiBundler.sol:

• The constructor of the contract does not validate that the _permittedNfts parameter is not a zero address.
• The constructor of the contract does not validate that the _airdropFlashLoan parameter is not a zero address.

contracts/ImmutableBundle.sol:

• The constructor of the contract does not validate that the _bundler parameter is not a zero address.
• The constructor of the contract does not validate that the _personalBundlerFactory parameter is not a zero address.

contracts/PersonalBundlerFactory.sol:

• The constructor of the contract does not validate that the _personalBundlerImplementation parameter is not a zero address.

contracts/ERC998TopDown.sol:

• The childContractByIndex function does not validate that the _index parameter is a valid index.
• The childTokenByIndex function does not validate that the _index parameter is a valid index.

contracts/utils/Ownable.sol:

• The constructor of the contract does not validate that the _initialOwner parameter is not a zero address.

RISK ACCEPTED: The NFTfi team accepted the risk of this finding.

Inline assembly is a way to access the Ethereum Virtual Machine at a low level. This discards several important safety features of Solidity and the static compiler. Because the EVM is a stack machine, it is often hard to address the correct stack slot and provide arguments to opcodes at the correct point on the stack. Solidity’s inline assembly tries to facilitate that and other issues arising when writing manual assembly. Assembly is much more difficult to write because the compiler does not perform checks, so the contract developer should be aware of this warning.

Code Location

ERC998TopDown.sol

assembly {
    parentTokenOwner := or(ERC998_MAGIC_VALUE, parentTokenOwnerAddress)
}

ERC998TopDown.sol

assembly {
    rootOwner := or(ERC998_MAGIC_VALUE, rootOwnerAddress)
}

ERC998TopDown.sol

assembly {
    tokenId := mload(add(_data, 0x20))
}

ACKNOWLEDGED: The NFTfi team acknowledged this issue.

Multiple gas cost optimization opportunities were identified in the loops of the NftfiBundler contract:

• Unnecessary reading of the array length on each iteration wastes gas.

• Using != consumes less gas than <.

• It is possible to further optimize loops by using unchecked loop index incrementing and decrementing.

• Loop counters do not need to be set to 0, since uint256 is already initialized to 0.

Code Location

contracts/NftfiBundler.sol

• Line 180 for (uint256 i = 0; i < _bundleElements.length; ++i) {
• Line 193 for (uint256 j = 0; j < _bundleElements[i].ids.length; ++j) {
• Line 192 for (uint256 j = 0; j < _bundleElements[i].ids.length; ++j) {
• Line 204 for (uint256 i = 0; i < _bundleElements.length; ++i) {
• Line 206 for (uint256 j = 0; j < _bundleElements[i].ids.length; ++j) {

contracts/PermittedNFTs.sol

• Line 120 for (uint256 i = 0; i < _nftContracts.length; ++i) {

SOLVED: The NFTfi team implemented the recommended gas optimizations.

Commit ID: d93033e7d122168797981dfbd439374fbe5d4dd2

The scoped contracts have configured the fixed pragma set to 0.8.4. The latest solidity compiler version, 0.8.17, fixed important bugs in the compiler along with new native protections. The current version is missing the following fixes: 0.8.5, 0.8.6, 0.8.7, 0.8.8, 0.8.9, 0.8.12, 0.8.13, 0.8.14, 0.8.15, 0.8.16, 0.8.17.

The official Solidity's recommendations are that you should use the latest released version of Solidity when deploying contracts. Apart from exceptional cases, only the newest version receives security fixes.

SOLVED: The NFTfi team bumped the Solidity compiler version to 0.8.17.

Commit ID: faa56c0d56293a7a43008a4c2f4f2500ba131cbf

Instead of using the `&&`` operator in a single require statement to check multiple conditions, using multiple require statements with one condition per require statement saves 8 GAS per operation. The gas difference can only be realized if the revert condition is satisfied.

Code Location

ImmutableBundle.sol

    /**
     * @notice used by the owner account to be able to drain ERC721 tokens received as airdrops
     * for the locked  collateral NFT-s
     * @param _tokenAddress - address of the token contract for the token to be sent out
     * @param _tokenId - id token to be sent out
     * @param _receiver - receiver of the token
     */
    function rescueERC721(
        address _tokenAddress,
        uint256 _tokenId,
        address _receiver
    ) external onlyOwner {
        IERC721 tokenContract = IERC721(_tokenAddress);
        require(
            _tokenAddress != address(bundler) &&
                !PersonalBundlerFactory(personalBundlerFactory).personalBundlerExists(msg.sender),
            "token is a bundle"
        );
        require(tokenContract.ownerOf(_tokenId) == address(this), "nft not owned");
        tokenContract.safeTransferFrom(address(this), _receiver, _tokenId);
    }

The following tests were carried out in Remix with optimization turned both on and off

    require ( a > 1 && a < 5, "Initialized");
    return  a + 2;

Execution cost 21617 with optimization and using && 21976 without optimization and using &&

After splitting the require statement

    require (a > 1 ,"Initialized");
    require (a < 5 , "Initialized");
    return a + 2;

Execution cost 21609 with optimization and split require 21968 without optimization and using split require

SOLVED: The NFTfi team solved the issue by refactoring the mentioned require statement.

Commit ID: 52f68e41a729e83f27c1cb747a464a2367132d5b

The following library imports can be removed because they are redundant or not used in the contracts:

• IERC20.sol is also included in SafeERC20.sol.
• IERC1155.sol is not used in some contracts.
• ERC721Holder.sol is not used in some contracts.

Code Location

contracts/NftfiBundler.sol

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";

contracts/ImmutableBundle.sol

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155.sol";

``` {language=solidity firstnumber="7 " caption="contracts/PersonalBundler.sol"} import "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";

``` {language=solidity firstnumber="8" caption="contracts/airdrop/AirdropFlashLoan.sol"}
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

SOLVED: The NFTfi team solved the issue by removing unnecessary imports.

Commit ID: fef3ac4bb4eb87a78e43082a560365a767178aae

Users can add any whitelisted NFTs to their bundles or personal bundles with the safeTransferFrom() or getChild() functions.

However, it has been detected that no checks are in place to ensure that users can only add tokens to bundles they already own. Those kinds of checks are already implemented in functions such as sendElementsToPersonalBundler, in which the owner of any bundle can only send the tokens to a personal bundle they own:

NftfiBundler.sol

    /**
     * @notice Remove all the children from the bundle and send to personla bundler.
     * If bundle contains a legacy ERC721 element, this will not work.
     * @dev This method may run out of gas if the list of children is too big. In that case, children can be removed
     *      individually.
     * @param _tokenId the id of the bundle
     * @param _personalBundler address of the receiver of the children
     */
    function sendElementsToPersonalBundler(uint256 _tokenId, address _personalBundler) external {
        _validateReceiver(_personalBundler);
        _validateTransferSender(_tokenId);

        //fix this actual personalBundlerExists
        require(
            IERC165(_personalBundler).supportsInterface(type(IERC998ERC721TopDown).interfaceId),
            "has to implement IERC998ERC721TopDown"
        );
        uint256 personalBundleId = 1;
        //make sure sendeer owns personal bundler token
        require(IERC721(_personalBundler).ownerOf(personalBundleId) == msg.sender, "has to own personal bundle token");

        // In each iteration all contracts children are removed, so eventually all contracts are removed
        while (childContracts[_tokenId].length() > 0) {
            address childContract = childContracts[_tokenId].at(0);

            // In each iteration a child is removed, so eventually all contracts children are removed
            while (childTokens[_tokenId][childContract].length() > 0) {
                uint256 childId = childTokens[_tokenId][childContract].at(0);

                _removeChild(_tokenId, childContract, childId);

                try
                    IERC721(childContract).safeTransferFrom(
                        address(this),
                        _personalBundler,
                        childId,
                        abi.encodePacked(personalBundleId)
                    )
                {
                    // solhint-disable-previous-line no-empty-blocks
                } catch {
                    revert("only safe transfer");
                }
                emit TransferChild(_tokenId, _personalBundler, childContract, childId);
            }
        }
    }

This behavior allows the owner of the bundle to extract any token included in it, no matter who was the original owner. This can be used to use NFTfi reputation to perform phishing campaigns or any similar malicious activity that might have a reputational impact on the project.

**ACKNOWLEDGED: ** The NFTfi team acknowledged this issue.

Open To-dos can point to architecture or programming issues that still need to be resolved. Often these kinds of comments indicate areas of complexity or confusion for developers. This provides value and insight to an attacker who aims to cause damage to the protocol.

Code Location

NftfiBundler.sol

        //fix this actual personalBundlerExists

SOLVED: The NFTfi team solved the issue by removing every TODO present in the code.

Commit ID: d93033e7d122168797981dfbd439374fbe5d4dd2

Natspec documentation are useful for internal developers that need to work on the project, external developers that need to integrate with the project, auditors that have to review it but also for end users given that many chain explorers have officially integrated the support for it directly on their site.

It has been detected that, while many contracts have a complete natspec documentation, other contracts or functions are little to no documented.

SOLVED: The NFTfi team added the missing natspec documentation.

Commit IDs:

• 31f25502aeba5c2f623c70386619c28a3de5266e
• ae470d0473f24271e6b9471f9111b14a607f6270