SZNS’ BountyBoard: Unauthorized TransferFrom Vulnerability

August 18, 2022 by patrickd

Timeline

• Thursday, 4th August: Possible vulnerability noticed. Contacted sznsDAO Team via Discord while further investigating the suspicion by creating a local PoC. Determined that a single person is most at risk of losing significant funds through this vulnerability. Through the ENS name, it's clear that this person is part of the SZNS Team, the person was directly contacted and removed approval to the vulnerable contract. A meeting with the Team was scheduled.
• Friday, 5th August: The PoC is presented to the Team during the meeting. Team was recommended to first warn a small circle and slowly increase the reach to ensure that affected users are reached first and potential malicious actors from outside of the community won't hear from it too soon. A list of likely affected addresses, of which some have ENS names, is provided to the Team. They'll work on trying to identify affected users and contact them directly. Although no official Bug Bounty program exists, the Team offers 1 ether as a reward, which is gladly accepted.
• Monday, 8th August: Got confirmation that the web Interface for interacting with the vulnerable contract was disabled.
• Sunday 14th August: All individuals that could be identified were contacted and warned to revoke approvals.
• Monday 15th August: A warning to remove approvals from the affected contract (without further explanation) was posted on the SZNS Discord.
• Friday 19th August: Vulnerability is publicly disclosed.

The SZNS Team also followed general improvement recommendations:

• A security specific contact (opens in a new tab) for disclosing security issues was added to the documentation.
• Specific Bug Bounty rules (opens in a new tab) were adopted and added to the documentation.

The documentation still claims that "SZNS largely relies on battle-tested smart contracts that have been audited by Peckshield", but it does not explain when these audits happened and for which parts of the SZNS ecosystem they were done. It's recommended to publish the original reports for transparency.

Vulnerability

The BountyBoard contract allows owners of ERC20 Token Contracts with Minting capability to exchange their Tokens for NFTs. They can create an Order describing what NFTs they are looking for and owners of said NFTs can fill these Orders, giving away their NFT in exchange for freshly minted ERC20 Tokens.

    struct Order {
        IERC721Validator validator;     // Controls what NFTs are accepted.
        ERC20PresetMinterPauser erc20;  // The reward ERC20 token.
        address nftBeneficiary;         // The receiver of NFTs.
        uint256 tokensPerTribute;       // The amount of ERC20 tokens for each NFT.
        uint256 expirationTime;
    }
 
    ...
 
    /// @notice Adds an order to the "BountyBoard" orderbook
    /// @param order Struct of all the order params that stay static.
    /// @param maxFills Maximum number of NFTs this order can be filled for
    /// @return orderHash The unique identifying hash for the added order
    function addOrder(Order calldata order, uint256 maxFills)
        external
        returns (bytes32 orderHash)
    {
        ...

The issue is that the function allowing to fill these orders does not check whether the caller owns the NFTs that they want to fill the order with. This means that the order can be filled with NFTs of other people who still own NFTs for a collection they approved the BountyBoard contract for.

    function batchFillOrder(
        Order calldata order,
        ERC721Grouping[] calldata erc721Groupings
    ) internal {
            ...
                if (!order.validator.meetsCriteria(address(erc721), id)) {
                    revert InvalidNftError(address(erc721), id);
                }
                // Forward NFT to benecifiary
                // NOTE: reentrancy should be safe here, since we're decrementing
                //   the number of fills based on its later value.
                erc721.safeTransferFrom(
                    erc721.ownerOf(id),
                    order.nftBeneficiary,
                    id
                );
            }
        }
        // Should throw if underflow
        remainingFills[orderHash] -= tributeCounter;
        // Payout to order filler
        order.erc20.mint(msg.sender, order.tokensPerTribute * tributeCounter);
    }

The issue should easily be fixed by replacing erc721.ownerOf(id) with msg.sender. That way attempting to transfer the NFTs of other users will error since the callee does not own them.

Attack Scenarios

Two ways by which this vulnerability could have been exploited were identified.

Reward Stealing

If the attacker is more interested in one of the offered ERC20 Tokens than any of the approved NFTs, they could "tribute" approved NFTs of other people to obtain the reward.

1. Bob created an order on the BountyBoard offering their ERC20 Tokens in exchange for specific NFTs
2. Alice has multiple NFTs from the collection Bob desires and decides to exchange one of them
3. Alice gives approval to the BountyBoard contract, effectively making it an operator over the NFTs she owns of the collection
4. Alice fills Bob's order giving one of her NFTs in exchange for Bob's ERC20 Tokens
5. Eve notices Bob's order and is interested in obtaining the ERC20 Tokens but does not own the required NFTs to participate
6. Eve notices the vulnerability in the BountyBoard contract
7. Eve notices that Alice gave approval to the BountyBoard contract to manage all of her NFTs for the collection Bob is interested in, and that Alice owns more of said NFTs that she has not exchanged for Tokens yet
8. Eve can call the order filling function while specifying Alice's NFTs as tribute
9. Eve will receive Bob's ERC20 tokens as a reward, Bob receives Alice's NFTs
10. Alice has lost all NFTs of the collection that she approved the BountyBoard to manage

NFT Stealing

If the attacker is interested in obtaining all NFTs that have been approved to the contract, they are able to create an Order with a worthless custom ERC20 Token and have the validator accept any ERC721 Tokens as meeting the criteria for the Order. Then the attacker can batch-fill their own order to obtain all of the NFTs they determined to be available for transfer.

1. Bob created an order on the BountyBoard offering their ERC20 Tokens in exchange for specific NFTs
2. Alice has multiple NFTs from the collection Bob desires and decides to exchange one of them
3. Alice gives approval to the BountyBoard contract, effectively making it an operator over the NFTs she owns of the collection
4. Alice fills Bob's order giving one of her NFTs in exchange for Bob's ERC20 Tokens
5. Eve notices the vulnerability in the BountyBoard contract and is interested in obtaining Alice's NFTs
6. Eve creates an Order that accepts any NFT with herself as the beneficiary, in exchange for a worthless ERC20 Token she created
7. Eve fills her own order while specifying Alice's NFTs as tribute
8. Eve will receive Alice's NFTs, and also some of her own ERC20 Tokens
9. Alice has lost all NFTs of collections that she approved the BountyBoard to manage

Conclusion

The likelihood of the vulnerability being exploited at some point seemed almost certain, but luckily the severity at this point was very low. The most significant amounts of funds at risk were quickly taken to safety and by carefully warning the most likely affected users it was not exploited by bad actors.

Although this was the first security issue ever reported to them, the SZNS Team reacted openly and quickly to warnings and suggestions and showed that they intend to not only handle the current issue but want to improve processes for the future.