Platform: Code4rena
Start Date: 21/06/2022
Pot Size: $30,000 USDC
Total HM: 12
Participants: 96
Period: 3 days
Judge: HardlyDifficult
Total Solo HM: 5
Id: 140
League: ETH
Rank: 4/96
Findings: 2
Award: $2,368.15
🌟 Selected for report: 1
🚀 Solo Findings: 1
🌟 Selected for report: unforgiven
2339.3444 USDC - $2,339.34
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
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.
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:
Basket with well known NFT token list. let's assume the Basket name is Basket_MNibblVaultFactory for Basket_M id=0 token.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.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.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.Vault_M by calling buy() function.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()Vault_M .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.
VIM
check ownership of id=0 in every iteration or don't allow ownership transfer in the multiple-token-transfer functions
#0 - GalloDaSballo
2022-06-26T22:06:10Z
Contract checks if you own it as owner of Basket has bought it, and as such is entitled to underlying tokens
#1 - KenzoAgada
2022-06-27T00:33:26Z
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.
#2 - HardlyDifficult
2022-07-03T12:11:48Z
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.
#3 - GalloDaSballo
2022-07-03T15:50:09Z
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)
#4 - HardlyDifficult
2022-07-03T15:56:32Z
The finding in essence is claiming that you can setup an empty
Basketand 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.
#5 - GalloDaSballo
2022-07-03T17:25:58Z
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
#6 - HardlyDifficult
2022-07-03T21:34:26Z
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.
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0x29A, 0x52, 0xNazgul, 0xNineDec, 0xc0ffEE, 0xf15ers, 0xkatana, BowTiedWardens, Chom, ElKu, Funen, GalloDaSballo, JC, JMukesh, JohnSmith, Lambda, Limbooo, MadWookie, MiloTruck, Nethermind, Noah3o6, Nyamcil, Picodes, PwnedNoMore, Randyyy, RoiEvenHaim, SmartSek, StErMi, Tadashi, TerrierLover, TomJ, Tomio, Treasure-Seeker, UnusualTurtle, Varun_Verma, Wayne, Waze, _Adam, apostle0x01, asutorufos, berndartmueller, c3phas, catchup, cccz, cloudjunky, codexploder, cryptphi, defsec, delfin454000, dipp, ellahi, exd0tpy, fatherOfBlocks, hansfriese, hyh, joestakey, kebabsec, kenta, masterchief, minhquanym, naps62, oyc_109, pashov, peritoflores, reassor, rfa, robee, sach1r0, saian, sashik_eth, shenwilly, simon135, slywaters, sorrynotsorry, sseefried, unforgiven, xiaoming90, ych18, zuhaibmohd, zzzitron
28.8052 USDC - $28.81
This is a classic Code4rena issue:
https://github.com/code-423n4/2021-04-meebits-findings/issues/2 https://github.com/code-423n4/2021-10-tally-findings/issues/20 https://github.com/code-423n4/2022-01-openleverage-findings/issues/75
The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:
The claimer smart contract does not implement a payable function. The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call’s gas usage above 2300. Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.
VIM
I recommend using call() instead of transfer().
#0 - mundhrakeshav
2022-06-26T10:14:32Z
#18
#1 - HardlyDifficult
2022-07-03T22:49:58Z
Agree that using .transfer is now discouraged. I think a difference here as compared to other contests is that the _to address is simply an input to this function call -- so if it reverts they could try again with a EOA and then transfer manually to the contract. Lowering risk and converting this into a QA report for the warden.
#2 - HardlyDifficult
2022-07-04T13:47:14Z
#3 - HardlyDifficult
2022-07-04T13:57:17Z