search

code-423n4 / 2021-12-nftx-findings

0 stars 0 forks source link

NFTXMarketplaceZap: Balance check can result in DOS #231

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

GreyArt

Vulnerability details

Impact

The _mint721() and _mint1155() functions check that the correct number of vault tokens have been minted, after accounting for mint fees.

uint256 balance = (count * BASE) - INFTXVault(vault).mintFee()*count;
require(balance == IERC20Upgradeable(vault).balanceOf(address(this)), "Did not receive expected balance");

This condition will not hold true if a malicious party sends in 1 wei of the vault token to the MarketplaceZap contract, resulting in a denial of service since the function will always revert.

Note that unlike the StakingZap contract, there isn’t rescue() function to remove any funds sent by the malicious party. Nevertheless, even with its existence, it could be cumbersome and costly to perform rescues (if we assume the owner is a multisig + ETH mainnet costs for the transaction).

The malicious party can also cause griefing by frontrunning a liquidity provision transaction.

Recommended Mitigation Steps

The balance comparison should be similar to the provideInventory721() and provideInventory1155() functions of the StakingZap contract.

The sample fix for _mint721() is provided below. The fix for _mint1155() is similar.

uint256 oldBal = IERC20Upgradeable(vault).balanceOf(address(this));
uint256 count = INFTXVault(vault).mint(ids, emptyIds); // replace emptyIds with amounts for _mint1155
uint256 balChange = (count * BASE) - (count * INFTXVault(vault).mintFee());
uint256 newBal = IERC20Upgradeable(vault).balanceOf(address(this));
require(newBal == oldBal + balChange, "Incorrect vtokens minted");
0xKiwi commented 2 years ago

Valid find, disagreeing with severity, confirming and marking as duplicate.

dmvt commented 2 years ago

Duplicate of #107