code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/SendValueWithFallbackWithdraw.sol#L37-L77

Vulnerability details

Impact

The NFTMarketFees contract and the NFTMarketReserveAuction contract use the _sendValueWithFallbackWithdraw function to send ether to FoundationTreasury, CreatorRecipients, Seller, Bidder. When the receiver fails to receive due to some reasons (exceeding the gas limit or the receiver contract cannot receive ether), it will record the ether to be sent in the pendingWithdrawals variable.

  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) {
      // Record failed sends for a withdrawal later
      // Transfers could fail if sent to a multisig with non-trivial receiver logic
      unchecked {
        pendingWithdrawals[user] += amount;
      }
      emit WithdrawPending(user, amount);
    }
  }

The user can then withdraw ether via the withdraw or withdrawFor functions.

  function withdraw() external {
    withdrawFor(payable(msg.sender));
  }
  function withdrawFor(address payable user) public nonReentrant {
    uint256 amount = pendingWithdrawals[user];
    if (amount == 0) {
      revert SendValueWithFallbackWithdraw_No_Funds_Available();
    }
    pendingWithdrawals[user] = 0;
    user.sendValue(amount);
    emit Withdrawal(user, amount);
  }

However, the withdrawFor function can only send ether to the address recorded in pendingWithdrawals. When the recipient is a contract that cannot receive ether, these ethers will be locked in the contract and cannot be withdrawn.

Proof of Concept

https://github.com/code-423n4/2022-02-foundation/blob/main/contracts/mixins/SendValueWithFallbackWithdraw.sol#L37-L77

Tools Used

None

Recommended Mitigation Steps

Add the withdrawTo function as follows:

  function withdrawTo(address payable to) public nonReentrant {
    uint256 amount = pendingWithdrawals[msg.sneder];
    if (amount == 0) {
      revert SendValueWithFallbackWithdraw_No_Funds_Available();
    }
    pendingWithdrawals[msg.sneder] = 0;
    to.sendValue(amount);
    emit Withdrawal(msg.sneder, amount);
  }

HardlyDifficult commented 2 years ago

We believe this is better classified as a 2 (Med Risk).

Worst case is funds for a user are trapped in escrow, however they are correctly attributed to that user and cannot be accessed by any other account. If we did not address this by launch, the issue could be corrected post launch via a contract upgrade and that user would then be made whole. So the funds are just temporarily unavailable to them.

This is a great report and we will be making a change to address the problem!

When reviewing the recommended mitigation we identified a new potential risk that could introduce. So we are going a slightly different way...

The _sendValueWithFallbackWithdraw function will send FETH tokens when it fails to transfer ETH. For any account that currently has a non-zero pendingWithdrawals we will include a migration function allowing us to move the escrowed ETH out of the market contract and into that user's FETH account.

Once that migration has been completed, we will delete the migration function and withdraw* functions -- simplifying the market contract and freeing up much needed contract size to make room for new features in the future.

alcueca commented 2 years ago

The combination of upgradability with the limited scope of the vulnerability makes it reasonable to downgrade this to a medium severity.

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