Reentrancy bug in Basket's withdraw multiple tokens function which gives attacker ability to transfer basket ownership and spend it but withdraw all the tokens out of basket

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L41-L47 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L68-L75 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/Basket.sol#L91-L97

Vulnerability details

Impact

Basket is used for keep multiple tokens in contract and mint one NFT token to represent their ownership. Basket only allows for owner of NFT(id=0) to withdraw tokens from Basket address. users can deposit multiple tokens in one Basket and then create a NibbVault based on that Basket NFT. but due to reentrancy vulnerability in Basket it's possible to call the multiple-token-withdraw functions (withdrawMultipleERC721(), withdrawMultipleERC1155(), withdrawMultipleERC721() and withdrawMultipleERC20()) and in the middle their external calls, spend Basket NFT (transfer ownership of id=0 to other contract, for example createVault()) and receive some fund from other, then in the rest of the multiple-token-withdraw function withdraw all the basket tokens. Basket shouldn't allow transferring ownership of id=0 in the middle of multiple token withdraws.

Proof of Concept

This is withdrawMultipleERC721() code:

    function withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) external override {
        require(_isApprovedOrOwner(msg.sender, 0), "withdraw:not allowed");
        for (uint256 i = 0; i < _tokens.length; i++) {
            IERC721(_tokens[i]).safeTransferFrom(address(this), _to, _tokenId[i]);
            emit WithdrawERC721(_tokens[i], _tokenId[i], _to);
        }
    }

As you can see, contract only checks the ownership of id=0 in the beginning of the function to see that user allowed to perform this action or not. then it iterates through user specified addresses and call safeTransferFrom() function in those address by user specified values. the bug is that in the middle of the external calls attacker can spend Basket NFT id=0 (give ownership of that basket to other contracts and receive fund from them, for example attacker can call createVault in NibblVaultFactory and create a vault and call other contracts to invest in that vault) then in the rest of the iterations in withdrawMultipleERC721() attacker can withdraw Basket tokens. so even so the ownership of the Basket has been transferred and attacker received funds for it, attacker withdraw Basket tokens too. This is the steps attacker would perform:

1. create a Basket with well known NFT token list. let's assume the Basket name is Basket_M
2. give approve permission to NibblVaultFactory for Basket_M id=0 token.
3. call Basket_M.withdrawMultipleERC721(address[] memory _tokens, uint256[] memory _tokenId, address _to) with list of all the tokens in basket to withdraw all of them, but the first address in the _tokens list is the address that attacker controls.
4. Basket_M would check that attacker is owner of the basket (owner of the id = 0) and in first iteration of the for it would call attacker controlled address which is a contract that attacker wrote its code.
5. attacker contract would call NibblVaultFactory.createVault() with Basket_M address and id=0 to create a vault which then transfer the ownership of Basket_M id=0 to the vault address. let's assume it's Vault_M.
6. attacker contract would buy some fraction of Vault_M by calling buy() function.
7. let's assume there are other contracts(call it Invest_Contract) that would want to buy fraction of the well known NFTs in the basket and Invest_Contract invest some fund in vault having those NFT in vault's address or vault's basket just by calling Invest_Contract. attacker contract would call Invest_Contract to invest in Vault_M and Invest_Contract would check that well known NFT is in Basket_M id=0 which belongs to Vault_M to it would invest money on it by calling initiateBuyout()
8. attacker contract then withdraw his money from Vault_M .
9. the rest of Basket_M.withdrawMultipleERC721() for iterations performs and all the NFT tokens of the Basket_M would be send to attacker and Basket_M would have nothing.

steps 5 to 8 can be other things, the point is in those steps attacker would spent Basket_M and receive some fund from other contract while those other contracts checks that they are owner of the Basket_M which has well known NFT tokens, but in fact attacker withdraw those well known NFT tokens from Basket_M after spending it in the rest of the withdrawMultipleERC721() iterations. (those above step 5-8 is just a sample case)

So Basket shouldn't allow ownership transfer in the middle of the Basket_M.withdrawMultipleERC721() and similar multiple-token-withdraw functions or it should check the ownership in every iteration.

Tools Used

VIM

Recommended Mitigation Steps

check ownership of id=0 in every iteration or don't allow ownership transfer in the multiple-token-transfer functions

[GalloDaSballo]

Contract checks if you own it as owner of Basket has bought it, and as such is entitled to underlying tokens

[KenzoAgada]

The attack is contingent on a regular user, creating a smart contract, which allows anybody to call it, which checks that a parameter-supplied Nibbl vault contains a Nibbl basket which contains a specific NFT, and then proceeds to buyout/buy shares of that vault.

Honestly it seems like the vector of attack is possible but quite far fetched.

[HardlyDifficult]

Creative thinking, this is what I'm here for!

If I'm following the flow correctly.. you kick off a bulk withdraw from the basket, the first NFT in the list is a malicious contract which then creates a vault for that basket (so the contract needs to be the basket owner, which is okay). Now the vault is fully created for that basket which still has valuable NFTs in it but you're mid-tx. Your malicious contract pings other contracts which can be prompted to ape in via on-chain logic -- their logic confirms all looks well and buys. But then control returns to the original batch withdrawal and the basket is drained.

Honestly it seems like the vector of attack is possible but quite far fetched.

I think I'd agree. To put it in terms of risk, this is not High: "valid attack path that does not have hand-wavy hypotheticals" -- this sounds a bit hand-wavy. Namely because it assumes Invest_Contract allows any address to trigger a purchase using other users funds which seems risky. And the victim here is Invest_Contract, not regular users of the protocol. Lowering to Medium. Great stuff though.

[GalloDaSballo]

The finding in essence is claiming that you can setup an empty Basket and sell it to external contracts, and those contracts would lose funds.

If that were the case the vulnerability would be in the "sniping / buying" contracts and not in the Basket nor the Vault.

The only thing the warden has shown is that they can create a Basket with a malicious token and through that they can call the Factory to create a Vault which after the tx will be empty.

This is logically equivalent to selling an empty vault, or selling a vault of BryptoPunks (typo on purpose, it's a scam token)

[HardlyDifficult]

The finding in essence is claiming that you can setup an empty Basket and sell it to external contracts, and those contracts would lose funds.

If that were the case the vulnerability would be in the "sniping / buying" contracts and not in the Basket nor the Vault.

The only thing the warden has shown is that they can create a Basket with a malicious token and through that they can call the Factory to create a Vault which after the tx will be empty.

This is logically equivalent to selling an empty vault, or selling a vault of BryptoPunks (typo on purpose, it's a scam token)

I agree in terms of normal usage. The key here is a 3rd party contract uses on-chain logic in order to authorize a purchase. If that were the case, while in the middle of the attack as described all checks that contract may perform would confirm assets were included and terms look good -- it would not be able to determine that the basket was in the middle of a batch withdraw request. Let me know if I'm overlooking something

The basket itself does not need to hold a malicious token -- the withdraw request takes an array of addresses, so the malicious contract only need to appear there.

[GalloDaSballo]

If that were the case, while in the middle of the attack as described all checks that contract may perform would confirm assets were included and terms look good -- it would not be able to determine that the basket was in the middle of a batch withdraw request. Let me know if I'm overlooking something

The 3rd party contract would need to check that the Basket is properly set via ownerOf(Basket) == Vault (where Vault is an address contained in the list of nibbledVaults from factory)

That would allow to determine if the contract is properly setup.

The basket itself does not need to hold a malicious token,

that is correct as you can setup any contract to accept the safeTransferFrom call.

My statement is that an automated 3rd party contract can get rekt, but that's not the contract under audit

[HardlyDifficult]

Agree that it's the 3rd party contract that suffers a loss.

An ownerOf(Basket) == Vault && ownerOf(NFT) == Basket check is insufficient here because if it's in the middle of this scenario then the owner checks will appear legit but by the end of the tx they won't be. That's the part that I'm still hung up on. Part of the Medium definition is "the function of the protocol or its availability could be impacted" -- although not an explicit goal, is it not implicit that protocols can be built upon with other contracts. The concern here seems to limit that ability, one could not build a contract that decided to participate based on on-chain state alone w/ or w/o an allow list of NFTs -- it would require a trusted actor to allow list specific vaults or to perform the action itself.

This is certainly grey though. Very hypothetical, e.g. it's not clear that a 3rd party contract would ever be interested in a capability like this. This is an interesting discussion! I'll sleep on it, but please chime in if you have more to add - I appreciate the feedback.