# NM-0070 Dyve

## Commit Hash:
https://github.com/Dyve-co/Contracts/tree/3711e842eef8671b707f396a3a7cd8c67e23b504

## General

## Dyve.sol

### [High] Reservoir flagged NFTs can be offered by a lender

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: When closing a position, a borrower must ensure that the NFT being returned is not flagged by Reservoir. This check is used to ensure that stolen NFTs cannot be used to close the position. Although the borrower has their NFT checked on return, the same check is not done for a lender when fulfilling the position. If a lender gets their flagged NFT order fulfilled by a borrower, the lender can now guarantee that they will either recieve a non-flagged NFT of the same collection or the order collateral (effectively laundering the stolen NFT).

Furthermore, Reservoir will flag an NFT that does not support the ERC721 standard. An example of an NFT that does not support the ERC721 standard is Cryptopunks. A lender could take advantage of this property on some NFT collections to create an order that cannot be closed by the borrower, guaranteeing that the lender will get the order collateral. This would mean that even if a borrower successfully shorted an NFT and wants to close the position to secure profits they would lose their collateral.

**Recommendation(s)**: Consider adding a Reservoir NFT flag check when fulfilling an order.

**Status**: Unresolved

**Update from the client**:

---

### [Medium] `premiumCollection` and `premiumTokenId` are not part of the signed order

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: When fulfilling an order the `premiumCollection` and `premiumTokenId` fields from the `OrderTypes.Order` struct are used to calculate the portion of order fees that should be taken by the Dyve protocol. Since these fields are not included in the order signature, the borrower is free to change these fields to any value. This can allow a malicious borrower to make a lender give a larger cut of the fee to the Dyve protocol by tampering with the order's premium collection and token fields.

Note that the order types `ERC721_TO_ERC20` and `ERC1155_TO_ERC20` are not affected by this as the lender is the one fulfilling the order, and they would not act in a way that negatively impacts their fees.

The code snippet below shows the function used for computing the hash of an order. Notice that `premiumCollection` and `premiumTokenId` are not used to compute the hash.

// keccak256("Order(uint256 orderType,address signer,address collection,uint256 tokenId,uint256 amount,uint256 duration,uint256 collateral,uint256 fee,address currency,uint256 nonce,uint256 endTime)")
bytes32 internal constant ORDER_HASH = 0xaad599fc66ff6b968ccb16010214cc3102e0a7e009000f61cab3f208682c3088;

...

function hash(Order memory order) internal pure returns (bytes32) {
    return keccak256(
        abi.encode(
            ORDER_HASH,
            order.orderType,
            order.signer,
            order.collection,
            order.tokenId,
            order.amount,
            order.duration,
            order.collateral,
            order.fee,
            order.currency,
            order.nonce,
            order.endTime
        )
    );
}

**Recommendation(s)**: Ensure that the `premiumCollection` and `premiumTokenId` fields are signed when fulfilling an order to prevent fee tampering.

<!--
Ensure that the `premiumCollection` and `premiumTokenId` fields are signed for the order when the order maker is the lender. *OR* Ensure that the lender can choose the `premiumCollection` and `premiumTokenId` used for fulfilling the order.
-->

**Status**: Unresolved

**Update from the client**:

---

<!-- ### [Medium] `premiumCollection` and `premiumTokenId` are not part of the signed `order`

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: The fields `premiumCollection` and `premiumTokenId` are used to define how much from the `fee` is taken by the Dyve protocol. These fields are not signed by order maker. In the cases where the order maker is the lender, this would allow the borrowers to choose the `premiumCollection` and `premiumTokenId` to use. The borrowers could decide if the lenders are affected by the `protocolFee` even if they have `premiumTokens`.

**Recommendation(s)**: TBD

**Recommendation(s) suggestion**:
The order hash must include the `premiumCollection` and the `premiumTokneId` fields when the lender is the order maker.
This could be achieved by adding another hashing function in `OrderType.sol`. During order fulfillment, the signature has to be checked against this hash.

Example of implementation:

//OrderTypes.sol
    function hashWithPremium(Order memory order) internal pure returns (bytes32) {
        return
            keccak256(
                abi.encode(
                    ORDER_HASH,
                    order.orderType,
                    order.signer,
                    order.collection,
                    order.tokenId,
                    order.amount,
                    order.duration,
                    order.collateral,
                    order.fee,
                    order.currency,
                    order.premiumCollection, //must not be changed by the order taker
                    oreder.premiumTokenId, //must not be changed by the order taker
                    order.nonce,
                    order.endTime
                )
            );
    }

//Dyve.sol
function fulfillOrder(OrderTypes.Order calldata order) {
[...]
    bytes32 orderHash
    //If the lender is the order maker
    if (order.orderType == Order.ERC721_TO_ERC20 ||
        order.orderType == Order.ERC1155_TO_ERC20) {
        orderHash = order.hashWithPremium();
    }
    else if (order.orderType == ETH_TO_ERC721 ||
            order.orderType ==  ETH_TO_ERC1155||
            order.orderType ==  ERC20_TO_ERC721||
            order.orderType ==  ERC20_TO_ERC1155
            ) {
    orderHash = order.hash();
    }
    else {
        revert InvalidOrderType();
    }
[...]
}

**Status**: Unresolved

**Update from the client**:

--- -->

### [Medium] Third party oracle dependency

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: The Dyve protocol makes use of the a [Resevoir](https://reservoir.tools/) API to determine whether an NFT was stolen before accepting it when closing a position. In the function `closePosition` the `message` argument (which comes from the API) must match the `RESEVOIR_ORACLE_ADDRESS`. This is shown below:

if (!ReservoirOracle._verifyMessage(messageId, maxMessageAge, message)) {
    revert ReservoirOracle.InvalidMessage();
}

It is not possible to change `RESEVOIR_ORACLE_ADDRESS` once it has been set. In the case that the API is discontinued or experiences downtime, it will be impossible to close orders as there will be no way to generate a valid message that can pass the message verification in `closePosition`. This would effectively act as a DOS against normally closing a position, leading to the borrower always losing their collateral.

**Recommendation(s)**: Consider adding a function that allows the `RESEVOIR_ORACLE_ADDRESS` variable to be changed. This way, in the case that the API has issues it will still be possible to change the oracle address so orders can still be closed.

**Status**: Unresolved

**Update from the client**:

---

### [Low] Protocol can receive no fees due to integer division

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: The functions `_transferETH` and `_transferERC20` determine a `protocolFee`, which is the amount of tokens that should be sent to the `protocolFeeRecipient` address. In both functions, `protocolFee` is calculated as follows:

uint256 protocolFee = (fee * protocolFeeManager.determineProtocolFeeRate(premiumCollection, premiumTokenId, to)) / 10000;

It is possible for the left-hand side of the integer division to be a number smaller than `10000`. In this case integer division would result in the `protocolFee` being zero. An example would be `9999 / 10000 == 0`. The potential fee amount lost is insignificant for Ether transfers and other 18 decimal ERC20 tokens, however for some tokens with a smaller number of decimals the fee loss may be higher. An example of such a token is [WBTC](https://etherscan.io/address/0x2260fac5e5542a773aa44fbcfedf7c193bc2c599) with 8 decimals, or [GUSD](https://etherscan.io/address/0x056Fd409E1d7A124BD7017459dFEa2F387b6d5Cd) with 2 decimals.

A lender aiming to take advantage of this to avoid protocol fees would have to set their `fee` to a value that multiplies with their fee rate in such a way that it remains under 10000. This constraint forces the lender to charge very little for fees and isn't economically viable to take advantage of.

**Recommendation(s)**: This finding is only concerned with fee loss for high value, low decimal ERC20 tokens. For the vast majority of tokens this will not be an issue as the potential losses are virtually non-existent and there is no incentive for a lender to act in this way. When whitelisting a currency for use within the protocol, consider the value per token and decimal amount.

**Status**: Unresolved

**Update from the client**:

---

### [Low] Resevoir oracle message replay attack

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: When a user closes a position with `closePosition` they must provide an argument `message` which contains information indicating whether an NFT is flagged. There is a time window of 5 minutes from when the message was created where it is considered valid. It is possible for a position to be closed, then the same NFT immediately be put up for lending again. A second borrow could occur and the borrower would be able to potentially return the NFT using the previous message even if the NFT had been flagged during the second borrowers possession. This could allow a borrower to return an NFT that has been flagged back to the lender.

**Recommendation(s)**: Consider adding the order hash to the message to ensure that messages are unique between orders.

**Status**: Unresolved

**Update from the client**:

---

### [Info] Non-existent orders have a status of `BORROWED`

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: Every order has a `status` field with the type `enum OrderStatus`. The first value declared in an enum is equivalent to `0`, the second is equivalent to `1` and so on. The first enum value in `OrderStatus` is `BORROWED`. Since unwritten state defaults to zero, any queries to a non-existent order will have a `status` of `BORROWED`. This could cause issues when interacting with other smart contracts and/or off-chain monitoring. The declaration of the `OrderStatus` enum is shown below:

// The NFT's listing status
enum OrderStatus {
    BORROWED, 
    EXPIRED,
    CLOSED
}

**Recommendation(s)**: Consider changing the `OrderStatus` enum so that its first (default) value indicates a non-existent order.

**Status**: Unresolved

**Update from the client**:

---

### [Info] Lender can borrow from themselves

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: It is possible for a lender to borrow from their own order. This can lead to unexpected behavior as it is possible for a user to create and then interact with an order within the same transaction. Functions that rely on `block.timestamp` may assume that some time has passed between function calls which may not be the case.

**Recommendation(s)**: A lender borrowing from their own order is not an expected user action and may lead to unexpected behavior within the protocol. Consider adding a check to prevent a lender and borrower from being the same address.

**Status**: Unresolved

**Update from the client**:

---

### [Info] Missing input validation for order duration

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: It is possible for a lender to create a valid order where the duration is zero. Although it does not make sense for a borrower to fulfill this order, as mentioned in a previous finding it is possible for a lender to borrow from their own order allowing for positions to be opened and then collateral claimed within the same transaction which may lead to unexpected behavior.

**Recommendation(s)**: Consider adding a check to ensure that `OrderTypes.Order.duration` cannot be zero when fulfilling an order.

**Status**: Unresolved

**Update from the client**:

---

### [Best Practice] The `nonReentrant` modifier should be added to all order management functions

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: The function `fulfillOrder` has the `nonReentrant` modifier, however the functions `closePosition` and `claimCollateral` do not. Under normal use cases the user would not be expected to reenter these functions, so adding `nonReentrant` helps to prevent undefined behavior that could potentially lead to unexpected results.

**Recommendation(s)**: Consider adding the `nonReentrant` modifier to the functions `closePosition` and `claimCollateral`.

**Status**: Unresolved

**Update from the client**:

---

### [Best Practice] Mapping entry can be saved to memory

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: In the functions `closePosition` and `claimCollateral` the order data is loaded as follows:

Order storage order = orders[orderHash];

Since the mapping is loaded to a `storage` variable rather than a `memory` variable, accessing any elements inside the struct will incur an SLOAD operation which can be expensive. The variable `order` can be changed to `memory` instead, which will reduce gas costs when accessing elements.

It should be noted that changing the order status of a memory variable would not cause a change in storage. Also, when emitting the event using the memory variable the `order` field will contain the unchanged `BORROWED` value, so the event emission would have to be changed as well.

An example is shown below for the function `claimCollateral`:

function claimCollateral(bytes32 orderHash) external {
- Order storage order = orders[orderHash];
+ Order memory order = orders[orderHash];

  // ...

- order.status = OrderStatus.EXPIRED;
+ orders[orderHash].status = OrderStatus.EXPIRED;

  // ...

  emit Claim(
    order.orderHash,
    order.orderType,
    order.borrower,
    order.lender,
    order.collection,
    order.tokenId,
    order.amount,
    order.collateral,
    order.currency,
-   order.status
+   OrderStatus.EXPIRED
  );
}

**Recommendation(s)**: Consider changing the variable `order` to `memory` to reduce storage queries to save gas.

**Status**: Unresolved

**Update from the client**:

---

### [Best Practice] Use of arbitrary number

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: In the function `cancelAllOrdersForSender`, the number `500000` is hardcoded. It is considered a best practice to define arbitrary values as constants.

<!-- Could also talk about the 10000 value in transferETH and transferERC20 but decided not to because it makes readability better -->

**Recommendation(s)**: Consider using a constant variable instead of arbitrary numbers.

**Status**: Unresolved

**Update from the client**:

---

<!--

### [Best Practice] `cancelMultipleMakerOrders` does not check the order status before cancelling it

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: `cancelMultipleMakerOrders` function does not check that the order exists or that order has already been canceled before canceling it. Any off-chain monitoring tool / application may have an inconsistent view of on-chain state.

**Recommendation(s)**: Before cancelling an order check that the order exists and is not already canceled.

**Status**: Unresolved

**Update from the client**:

---
-->

### [Best Practice] `_transferERC20` and `_transferETH` may transfer with no amount

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: In the functions `_transferERC20` and `_transferETH` it is possible to have a `protocolFee` of zero if the lender owns an NFT from a "premium collection". Even if the `protocolFee` is zero, a transfer to `protocolFeeRecipient` is still attempted.

In `_transferEth` the call `CALL` operation is still executed, costing unnecessary gas.

In `_transferERC20` on top of costing unnecessary gas, this can potentially lead to unexpected behavior as some ERC20 tokens may revert on zero transfers which would prevent an order from being fulfilled.

**Recommendation(s)**: Consider checking if the fee amount to be sent to the protocol is zero and if so, skip the transfer.

**Status**: Unresolved

**Update from the client**:

---

### [Best Practice] Hardcoded values for Interface ID

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: Writing magic values manually is always error prone. The `Dyve` contract has the `interfaceId` values for `EIP-721` and `EIP-1155` manually written as can be seen below.

bytes4 public constant INTERFACE_ID_ERC721 = 0x80ac58cd;
bytes4 public constant INTERFACE_ID_ERC1155 = 0xd9b67a26;

A typo in these values could causes integration issues to the protocol.

**Recommendation(s)**: Consider using the syntax `type(Type).interfaceId` instead of manually writing the values

- bytes4 public constant INTERFACE_ID_ERC721 = 0x80ac58cd;
- bytes4 public constant INTERFACE_ID_ERC1155 = 0xd9b67a26;
+ bytes4 public constant INTERFACE_ID_ERC721 = type(IERC721).interfaceId;
+ bytes4 public constant INTERFACE_ID_ERC1155 = type(IERC1155).interfaceId;

**Status**: Unresolved

**Update from the client**:

---

### [Best Practices] Code does not match specification

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**: In the definition of the `Dyve.Order` struct there is inline documentation (shown below) stating that the `Order.amount` field should always be set to `1` in the case of an ERC721 order. However, there are no checks in the code to ensure this condition.

struct Order {
    ...
    uint256 tokenId; // id of the token
    uint256 amount; // amount of the token (only applicable for ERC1155, always set to 1 for ERC721)
    uint256 duration; // duration of the borrow
    ...
}

A similar situation occurs for the `currency` field. The documentation provided by the Dyve team describes the `currency` field as follows: *"The type of ERC20 token to be used (zero address if native ETH is to be used)"*. However, this behavior does not exist in the code.

**Recommendation(s)**: Ensure that the code and documentation are consistent.

**Status**: Unresolved

**Update from the client**:

<!--

---

## ProtocolFeeManagement.sol

### [Info] `protocolFee` can be set to a value bigger than `100%` and cause all the future calls to `fulfillOrder` to revert

**File(s)**: [`contracts/ProtocolFeeManagement.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/ProtocolFeeManagement.sol)

**Description**: When fulfilling an order the functions `_transferETH` and `_transferERC20` compute how much from the `fee` belongs to Dyve protocol and will send the rest to the lender. This can be seen in the snippet of code below.

function _transferETH(...) internal {
    uint256 protocolFee =
        (fee * protocolFeeManager.determineProtocolFeeRate(premiumCollection, premiumTokenId, to)) / 10000;
    bool success;

    // 1. Protocol fee transfer
    (success,) = payable(protocolFeeRecipient).call{value: protocolFee}("");
    require(success, "Order: Protocol fee transfer failed");

    // 2. Lender fee transfer
    (success,) = payable(to).call{value: fee - protocolFee}("");
    require(success, "Order: Lender fee transfer failed");
}

function _transferERC20(...) internal {
    uint256 protocolFee =
        (fee * protocolFeeManager.determineProtocolFeeRate(premiumCollection, premiumTokenId, to)) / 10000;

    // 1. Protocol fee transfer
    IERC20(currency).safeTransferFrom(from, protocolFeeRecipient, protocolFee);

    // 2. Lender fee transfer
    IERC20(currency).safeTransferFrom(from, to, fee - protocolFee);

    // 3. Collateral transfer
    IERC20(currency).safeTransferFrom(from, address(this), collateral);
}

The functions `updateProtocolFeeRate(...)` and `updateCollectionFeeRate(...)` are used for setting the protocol fee rate. These functions do not apply any checks on the received value, as can be seen in the snippet of code below.

function updateCollectionFeeRate(...) external onlyOwner {
    premiumCollections[collection] = feeRate;

    emit UpdatedCollectionFeeRate(collection, feeRate);
}

function updateProtocolFeeRate(...) external onlyOwner {
    protocolFeeRate = _protocolFeeRate;

    emit UpdatedProtocolFeeRate(_protocolFeeRate);
}

This allows a fee rate bigger than `100%` to be set. Setting the fee rate to a value bigger than `100%` would cause all the future calls to `fulfillOrder(...)` to revert because of the substraction in `_transferETH(...)` and `_transferERC20(...)`.

**Recommendation(s)**: Consider adding a reasonable fee rate bound in the `updateCollectionFeeRate(...)` and `updateProtocolFeeRate(...)` functions.

**Status**: Unresolved

**Update from the client**:

---

### [Severity] Title

**File(s)**: [`contracts/Dyve.sol`](https://github.com/Dyve-co/Contracts/blob/3711e842eef8671b707f396a3a7cd8c67e23b504/contracts/Dyve.sol)

**Description**:

**Recommendation(s)**:

**Status**: Unresolved

**Update from the client**:

---

-->