Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buyAndSwap1155WETH Does Not Work As Intended #45

Open
code423n4 opened this issue Dec 18, 2021 · 0 comments
Open

buyAndSwap1155WETH Does Not Work As Intended #45

code423n4 opened this issue Dec 18, 2021 · 0 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Handle

leastwood

Vulnerability details

Impact

The buyAndSwap1155WETH function in NFTXMarketplaceZap aims to facilitate buying and swapping ERC1155 tokens within a single transaction. The function expects to transfer WETH tokens from the msg.sender account and use these tokens in purchasing vault tokens. However, the _buyVaultToken call in buyAndSwap1155WETH actually uses msg.value and not maxWethIn. As a result, the function will not work unless the user supplies both WETH and native ETH amounts, equivalent to the maxWethIn amount.

Proof of Concept

https://github.com/code-423n4/2021-12-nftx/blob/main/nftx-protocol-v2/contracts/solidity/NFTXMarketplaceZap.sol#L284-L314

function buyAndSwap1155WETH(
  uint256 vaultId, 
  uint256[] memory idsIn, 
  uint256[] memory amounts, 
  uint256[] memory specificIds, 
  uint256 maxWethIn, 
  address[] calldata path,
  address to
) public payable nonReentrant {
  require(to != address(0));
  require(idsIn.length != 0);
  IERC20Upgradeable(address(WETH)).transferFrom(msg.sender, address(this), maxWethIn);
  uint256 count;
  for (uint256 i = 0; i < idsIn.length; i++) {
      uint256 amount = amounts[i];
      require(amount > 0, "Transferring < 1");
      count += amount;
  }
  INFTXVault vault = INFTXVault(nftxFactory.vault(vaultId));
  uint256 redeemFees = (vault.targetSwapFee() * specificIds.length) + (
      vault.randomSwapFee() * (count - specificIds.length)
  );
  uint256[] memory swapAmounts = _buyVaultToken(address(vault), redeemFees, msg.value, path);
  _swap1155(vaultId, idsIn, amounts, specificIds, to);

  emit Swap(count, swapAmounts[0], to);

  // Return extras.
  uint256 remaining = WETH.balanceOf(address(this));
  WETH.transfer(to, remaining);
}

Tools Used

Manual code review.
Discussions with Kiwi.

Recommended Mitigation Steps

Consider updating the buyAndSwap1155WETH function such that the following line of code is used instead of this.

uint256[] memory swapAmounts = _buyVaultToken(address(vault), redeemFees, maxWethIn, path);
@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Dec 18, 2021
code423n4 added a commit that referenced this issue Dec 18, 2021
@0xKiwi 0xKiwi added resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") labels Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

2 participants