search

code-423n4 / 2022-08-foundation-findings

0 stars 0 forks source link

Minting NFTs can be reverted by multiple users during distribution of funds #177

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/SendValueWithFallbackWithdraw.sol#L34-L50

Vulnerability details

Impact

When a user purchases a NFT via NFTDropMarketFixedPriceSale.mintFromFixedPriceSale, funds are distributed via a call to MarketFees._distributeFunds. During this call, the following users are paid:

When paying each of these users, SendValueWithFallbackWithdraw._sendValueWithFallbackWithdraw is called which first makes a call to each user and if that fails, calls FETH.depositFor.

What's important here is that _sendValueWithFallbackWithdraw calls the user. For example:

// Entire function can be found here: https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/SendValueWithFallbackWithdraw.sol#L34-L50
//
(bool success, ) = user.call{ value: amount, gas: gasLimit }("");

Unfortunately, a user can revert a call made to them by modifying their contract to revert when funds are sent to the contract. This results in the following users below having the opportunity to halt a NFT sale via reverting calls made to their address:

Proof of Concept

The SendValueWithFallbackWithdraw._sendValueWithFallbackWithdraw function does not wrap the call in a try statement to catch the call reversion. Because of this, a revert when call is made will bubble up to NFTDropMarketFixedPriceSale.mintFromFixedPriceSale and prevent the NFT from being sold.

function _sendValueWithFallbackWithdraw(
  address payable user,
  uint256 amount,
  uint256 gasLimit
) internal {
  if (amount == 0) {
    return;
  }
  // Cap the gas to prevent consuming all available gas to block a tx from completing successfully
  // solhint-disable-next-line avoid-low-level-calls
  (bool success, ) = user.call{ value: amount, gas: gasLimit }("");
  if (!success) {
    // Store the funds that failed to send for the user in the FETH token
    feth.depositFor{ value: amount }(user);
    emit WithdrawalToFETH(user, amount);
  }
}

Tools Used

Text editor

Recommended Mitigation Steps

Foundation should handle cases where call reverts. If call reverts, then Foundation can utilize feth.DepositFor. To handle these cases, wrapping the call in a try statement will ensure that Foundation catches these reverts.

batu-inal commented 2 years ago

You can't try catch a call(). On a revert, call() returns a boolean of false which is handled by our code.

HardlyDifficult commented 2 years ago

Agree with @batu-inal here, if an external contract reverts when .call() is used -- it will return false instead of reverting the entire transaction. Our code handles this so that the minting will still complete successfully.