# (Draft) NiftySwapExchange Audit Report
*Provided by peppersec.com*

Repo link:
https://github.com/0xsequence/niftyswap PR #60

## Finding 1: ReentrancyGuard gas optimization
NiftySwap is using a version of reentrancy guard that uses boolean to keep track of the state: https://github.com/0xsequence/niftyswap/blob/master/src/contracts/utils/ReentrancyGuard.sol

OpenZeppelin implementation relies on uint256 constant instead for improved gas efficiency:
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol

### Recommendation
We recommend using uint256 constant for tracking state in ReentrancyGuard.sol for increased gas efficiency

## Finding 2: Flow control yielded across trust boundaries while on-chain data is in an inconsistent state

https://github.com/0xsequence/niftyswap/blob/master/src/contracts/exchange/NiftyswapExchange20.sol
in function _addLiquidity():99:

 // Mint liquidity pool tokens
    _batchMint(_provider, _tokenIds, liquiditiesToMint, "");

 // Transfer all currency to this contract
    TransferHelper.safeTransferFrom(currency, _provider, address(this), totalCurrency);

In the code above _batchMint() transfers liquidity tokens (ERC-1155), which also invokes onERC155Received() callback on the receiving contract, which is an untrusted party. Note that the callback is invoked after the tokens (ERC1155) were received and the contract state had been updated but before the corresponding currency (ERC20) was transferred to the exchange (NiftySwap) contract. Essentially, this order of operation yields control to an untrusted party after recalculating currency reserves but before transferring the currency, which leads to an inconsistent state. This is generally an anti-pattern and should be avoided.

This inconsistent state leads to the following consequences:

1. LP tokens are received by the caller but the corresponding currency is not yet transferred
2. function getCurrencyReserves() is a public view function that will report the result that doesn’t reflect the real state of reserves: the local state has been updated but the currency has not yet been received. The function is not guarded against reentrancy and any other function or token/contract relying on it may be vulnerable.
3. function getTotalSupply() will report the result before the currency is transferred to the exchange (NiftySwap). The function is not guarded against reentrancy and any other function or token/contract relying on it may be vulnerable.

This form of recurrency is sometimes called a light re-entrancy. A related fix has been introduced recently to OpenZeppelin's ERC1155Supply contract to guard against it:
https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2956

Furthermore, a similar issue was reported in a previous security audit (5.2):
https://github.com/0xsequence/niftyswap/blob/master/audits/consensys-diligence-audit-2020-02.pdf

But the interpretation of implications and recommendations were different.

### Recommendation
We recommend transferring the currency to the exchange first and then minting liquidity tokens, this will yield control to the receiver after collecting both tokens and currency. E.g.

// Transfer all currency to this contract
    TransferHelper.safeTransferFrom(currency, _provider, address(this), totalCurrency);

 // Mint liquidity pool tokens
    _batchMint(_provider, _tokenIds, liquiditiesToMint, "");

## Finding 3: Flow Control Yielded across trust boundaries while on-chain data is in an inconsistent state

NiftySwap does not guard against using ERC-777 in place of ERC-20. Upon transfer to ERC-777 contract, a callback is invoked on the receiver side thus yelling control across the trust boundaries.This may potentially open attack vectors similar to what was used in UniswapV1 attack: (https://github.com/OpenZeppelin/exploit-uniswap).

For example, if an attacker were to use ERC-777 as ERC-20 token then TransferHelper.safeTransfer(...) would invoke IERC777.transfer() followed by an attacker controlled onTokenReceive callback. This allows an attacker to stage a potential light form of re-entrancy attack against getters.

Consider _currencyToToken() function, line 148

 // Refund currency token if any
    if (totalRefundCurrency > 0) {
      TransferHelper.safeTransfer(currency, _recipient, totalRefundCurrency);
    }

    // Send Tokens all tokens purchased
    token.safeBatchTransferFrom(address(this), _recipient, _tokenIds, _tokensBoughtAmounts, "")

Chronologically the state updates are happening in the following order:

1. NiftySwapExchange20 contract received some currency
2. NiftySwapExchange20 updated currencyReserves contract state
3. NiftySwapExchange20 has not received the tokens. It calculates tokenReserves by querying token.balanceOf. To reiterate, currencyReserves were updated but not tokenReserves. This means the getter function calculating the price will report skewed results.
4. We process the refund of the currency first. Which means if the currency token is ERC-777, TransferHelper.safeTransfer() will eventually invoke a callback on the receiver yielding control to an untrusted party while the data state is inconsistent and price getters would report incorrect data.
5. After processing the refund NiftySwapExchange20 transfers tokens which may invoke onERC1155Received callback on the receiver. Price getters will report the correct result this time since tokens are transferred at this point.

There’s a potential for yielding the control twice where the first time price reported is skewed and the second time price reported is correct. In case other contracts are relying upon NiftySwap's price getters for price discovery, this potentially opens up a "light reentrancy" attack vector (into getter functions).

### Recommendation
Enforcing ERC-20 interface for the currency may be difficult.
We recommend clearly documenting the risk of using ERC-777 as a currency.

In _currencyToToken() function we recommend to swap line 149:

(TransferHelper.safeTransfer(currency, _recipient, totalRefundCurrency) )

and line 153:

token.safeBatchTransferFrom(address(this), _recipient, _tokenIds, _tokensBoughtAmounts, "");)

This way the token is transferred first and when receiver callbacks are invoked the prices are reported correctly.

## Finding 4: Total Supply Tracking Can Be Done Using Open Source Interface

Total LP supply in ERC1155 can be alternatively tracked using ERC1155Supply extension:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/extensions/ERC1155Supply.sol#L17

mapping(uint256 => uint256) private _totalSupply;

The mapping above in the extension may be used to replace the tracking of totalSupplies in the NiftySwapExchange20:

https://github.com/0xsequence/niftyswap/blob/master/src/contracts/exchange/NiftyswapExchange20.sol#L46

mapping(uint256 => uint256) internal totalSupplies;

### Recommendation
ERC-1155 Supply extension depends on OpenZeppelin ERC-1155 implementation, it invokes _beforeTokenTransfer callback. Please investigate if it makes sense to port ERC-1155 Supply to 0xsequence or consider using OpenZeppelin’s implementation.

## Finding 5: Unused Variable

NiftySwapExchange20 has variable shadowing by redefining the same var in the inner block of code in contracts/exchange/NiftyswapExchange20.sol:568:

      uint256 tokenReserve = tokenReserves[i];

      // Load total liquidity pool token supply for Token _id
      uint256 totalLiquidity = totalSupplies[id];
      require(totalLiquidity > 0, "NiftyswapExchange20#_removeLiquidity: NULL_TOTAL_LIQUIDITY");

      // Load currency and Token reserve's supply of Token id
      uint256 currencyReserve = currencyReserves[id];

      // Calculate amount to withdraw for currency  and Token _id
      uint256 currencyAmount;
      uint256 tokenAmount;

      {
        uint256 tokenReserve = tokenReserves[i];

### Recommendation
The variable declared in the outer scope is not being used and can be removed.

## Finding 6: Precision Loss in Royalty Calculation Using Global Fee

The goal of the PR in review is to increase precision in royalty calculations.
The code is using ROYALTIES_DENOMINATOR to perform royalty calculations by raising the precision to 4 decimal points and storing them in the royaltiesNumerator map.

In getRoyaltyInfo(..) function there’s a code branch that calculates the fee based on the globalRoyaltyFee. This function is a view and is returning lossy values due to the remainder being lost during integer division, see line NiftySwapExchange20.sol:876 (https://github.com/0xsequence/niftyswap/blob/next/src/contracts/exchange/NiftyswapExchange20.sol#L876) :

return (globalRoyaltyRecipient, (_cost.mul(globalRoyaltyFee)) / ROYALTIES_DENOMINATOR)

getRoyaltyInfo(...) view is called from other functions that rely on it to subsequently calculate hi-res royalty data, i.e. in _currencyToToken(...), NiftySwapExchange20.sol:155 (https://github.com/0xsequence/niftyswap/blob/next/src/contracts/exchange/NiftyswapExchange20.sol#L155) :

      (address royaltyRecipient, uint256 royaltyAmount) = getRoyaltyInfo(idBought, currencyAmount);
      if (royaltyAmount > 0) {
        royaltiesNumerator[royaltyRecipient] = royaltiesNumerator[royaltyRecipient].add(royaltyAmount.mul(ROYALTIES_DENOMINATOR));
      }

First, getRoyaltyInfo(...) is called which is lossy, it doesn’t keep the fractions due to integer division, then its value is increased in resolution and stored in a hi-res royaltiesNumerator map. To sum up, the use of lossy getRoyaltyInfo(...) for frequent operations using smaller currencies’ amounts would lead to losses in the amount of royalties paid out.

Here are other functions, which are similarly affected:
- _tokenToCurrency(...)
- _toRoundedLiquidity(...).

### Recommendation

Implement an internal method _getRoyaltyInfo(...) which would return a high-res royalty fee amount and use it for hi-res calculations instead. Existing public view getRoyaltyInfo(...) could simply invoke the internal _getRoyaltyInfo(...) method and divide its output by ROYALTIES_DENOMINATOR to return values consistent with the current implementation.