Merge pull request #234 from decentraland/feat/add-fingerprint-contra…

…ct-check

feat: Add fingerprinting estate contract check

src/logic/trades/utils.ts

@@ -1,11 +1,32 @@
 /* eslint-disable @typescript-eslint/naming-convention */
 import { Contract, TypedDataField, TypedDataDomain, verifyTypedData, toBeArray, zeroPadValue, JsonRpcProvider } from 'ethers'
-import { ChainId, ERC721TradeAsset, Network, TradeAsset, TradeAssetType, TradeCreation } from '@dcl/schemas'
+import { ChainId, ERC721TradeAsset, TradeAsset, TradeAssetType, TradeCreation } from '@dcl/schemas'
 import { ContractData, ContractName, getContract } from 'decentraland-transactions'
 import { InvalidECDSASignatureError, MarketplaceContractNotFound } from '../../ports/trades/errors'
 import { fromMillisecondsToSeconds } from '../date'
 import { hasECDSASignatureAValidV } from '../signatures'
 
+function getRPCUrlByChainId(chainId: ChainId): string {
+  let rpcPath: string
+  switch (chainId) {
+    case ChainId.ETHEREUM_MAINNET:
+      rpcPath = 'mainnet'
+      break
+    case ChainId.ETHEREUM_SEPOLIA:
+      rpcPath = 'sepolia'
+      break
+    case ChainId.MATIC_MAINNET:
+      rpcPath = 'polygon'
+      break
+    case ChainId.MATIC_AMOY:
+      rpcPath = 'amoy'
+      break
+    default:
+      throw new Error('Unsupported chainId')
+  }
+  return `https://rpc.decentraland.org/${rpcPath}`
+}
+
 export function getValueFromTradeAsset(asset: TradeAsset) {
   switch (asset.assetType) {
     case TradeAssetType.COLLECTION_ITEM:
@@ -117,29 +138,28 @@ export function isERC721TradeAsset(asset: TradeAsset): asset is ERC721TradeAsset
   return (asset as ERC721TradeAsset).tokenId !== undefined
 }
 
-async function getContractOwner(contractAddress: string, tokenId: string, network: Network, chainId: ChainId): Promise<string> {
+async function getContractOwner(contractAddress: string, tokenId: string, chainId: ChainId): Promise<string> {
   const abi = ['function ownerOf(uint256 tokenId) view returns (address)']
-  const RPC_URL = `https://rpc.decentraland.org/${
-    network === Network.ETHEREUM
-      ? chainId === ChainId.ETHEREUM_MAINNET
-        ? 'mainnet'
-        : 'sepolia'
-      : chainId === ChainId.MATIC_MAINNET
-      ? 'polygon'
-      : 'amoy'
-  }`
-  const provider = new JsonRpcProvider(RPC_URL)
+  const provider = new JsonRpcProvider(getRPCUrlByChainId(chainId))
   const contract = new Contract(contractAddress, abi, provider)
   return await contract.ownerOf(tokenId)
 }
 
-export async function validateAssetOwnership(
-  asset: ERC721TradeAsset,
-  signer: string,
-  network: Network,
-  chainId: ChainId
+export async function isEstateFingerprintValid(
+  contractAddress: string,
+  tokenId: string,
+  chainId: ChainId,
+  fingerprint: string
 ): Promise<boolean> {
+  const abi = ['function getFingerprint(uint256 tokenId) view returns (bytes32)']
+  const provider = new JsonRpcProvider(getRPCUrlByChainId(chainId))
+  const contract = new Contract(contractAddress, abi, provider)
+  const estateFingerprint = await contract.getFingerprint(tokenId)
+  return estateFingerprint.toLowerCase() === fingerprint.toLowerCase()
+}
+
+export async function validateAssetOwnership(asset: ERC721TradeAsset, signer: string, chainId: ChainId): Promise<boolean> {
   const { contractAddress, tokenId } = asset
-  const blockchainOwner = await getContractOwner(contractAddress, tokenId, network, chainId)
+  const blockchainOwner = await getContractOwner(contractAddress, tokenId, chainId)
   return blockchainOwner.toLowerCase() === signer.toLowerCase()
 }

src/ports/trades/component.ts

@@ -55,7 +55,7 @@ export function createTradesComponent(components: Pick<AppComponents, 'dappsData
       throw new InvalidTradeStructureError(trade.type)
     }
     // Validate if estate trade is correct
-    if (isEstateChain(trade.chainId) && !isValidEstateTrade(trade)) {
+    if (isEstateChain(trade.chainId) && !(await isValidEstateTrade(trade))) {
       throw new InvalidEstateTrade()
     }
 
@@ -65,7 +65,7 @@ export function createTradesComponent(components: Pick<AppComponents, 'dappsData
     }
 
     // validate right ownership
-    if (isERC721TradeAsset(trade.sent[0]) && !(await validateAssetOwnership(trade.sent[0], signer, trade.network, trade.chainId))) {
+    if (isERC721TradeAsset(trade.sent[0]) && !(await validateAssetOwnership(trade.sent[0], signer, trade.chainId))) {
       throw new InvalidOwnerError()
     }
 

src/ports/trades/utils.ts

@@ -14,6 +14,7 @@ import {
 } from '@dcl/schemas'
 import { fromTradeAndAssetsToEventNotification } from '../../adapters/trades/trades'
 import { getMarketplaceContracts } from '../../logic/contracts'
+import { isEstateFingerprintValid } from '../../logic/trades/utils'
 import { getBidsQuery } from '../bids/queries'
 import { getItemByItemIdQuery, getItemsQuery } from '../items/queries'
 import { DBItem } from '../items/types'
@@ -48,23 +49,23 @@ export function isEstateChain(chainId: ChainId): boolean {
   return chainId !== ChainId.MATIC_AMOY && chainId !== ChainId.MATIC_MAINNET
 }
 
-export function isValidEstateTrade(trade: TradeCreation): boolean {
+export async function isValidEstateTrade(trade: TradeCreation): Promise<boolean> {
   const contracts = getMarketplaceContracts(trade.chainId)
   const estateContract = contracts.find(contract => contract.name === 'Estates')
   if (!estateContract) {
     throw new EstateContractNotFoundForChainId(trade.chainId)
   }
 
-  // Check if the trade contains an estate with an empty extra field (all estates must have the extra field which is the fingerprint)
-  const isSentEstateTradeWrong = trade.sent.some(
-    asset => asset.contractAddress.toLowerCase() === estateContract.address.toLowerCase() && isBytesEmpty(asset.extra)
-  )
-  const isReceivedEstateTradeWrong = trade.received.some(
-    asset => asset.contractAddress.toLowerCase() === estateContract.address.toLowerCase() && isBytesEmpty(asset.extra)
-  )
-
-  if (isSentEstateTradeWrong || isReceivedEstateTradeWrong) {
-    return false
+  const assets = [...trade.sent, ...trade.received]
+  // Checks trades one by one to prevent unnecessary checks. This should be changed when implementing bundles or the cart system.
+  for (const asset of assets) {
+    // Only check if the asset is an estate
+    if (asset.contractAddress.toLowerCase() === estateContract.address.toLowerCase()) {
+      return (
+        !isBytesEmpty(asset.extra) &&
+        (await isEstateFingerprintValid(estateContract.address, (asset as ERC721TradeAsset)?.tokenId, trade.chainId, asset.extra))
+      )
+    }
   }
 
   return true

test/unit/trades-component.spec.ts

@@ -143,7 +143,7 @@ describe('when adding a new trade', () => {
   describe('when the trade structure is not valid for a given type', () => {
     beforeEach(() => {
       jest.spyOn(utils, 'validateTradeByType').mockResolvedValue(false)
-      jest.spyOn(utils, 'isValidEstateTrade').mockReturnValueOnce(true)
+      jest.spyOn(utils, 'isValidEstateTrade').mockResolvedValueOnce(true)
     })
 
     it('should throw an InvalidTradeStructureError', async () => {
@@ -155,7 +155,7 @@ describe('when adding a new trade', () => {
     beforeEach(() => {
       jest.spyOn(signatureUtils, 'validateTradeSignature').mockReturnValue(false)
       jest.spyOn(utils, 'validateTradeByType').mockResolvedValue(true)
-      jest.spyOn(utils, 'isValidEstateTrade').mockReturnValueOnce(true)
+      jest.spyOn(utils, 'isValidEstateTrade').mockResolvedValueOnce(true)
     })
 
     it('should throw an InvalidTradeSignatureError', async () => {
@@ -168,7 +168,7 @@ describe('when adding a new trade', () => {
       mockTrade.chainId = ChainId.ETHEREUM_SEPOLIA
       jest.spyOn(signatureUtils, 'validateTradeSignature').mockReturnValue(true)
       jest.spyOn(utils, 'validateTradeByType').mockResolvedValue(true)
-      jest.spyOn(utils, 'isValidEstateTrade').mockReturnValueOnce(false)
+      jest.spyOn(utils, 'isValidEstateTrade').mockResolvedValueOnce(false)
     })
 
     it('should throw an EstateTradeWithoutFingerprintError', async () => {
@@ -189,7 +189,7 @@ describe('when adding a new trade', () => {
     beforeEach(async () => {
       jest.spyOn(signatureUtils, 'validateTradeSignature').mockReturnValue(true)
       jest.spyOn(utils, 'validateTradeByType').mockResolvedValue(true)
-      jest.spyOn(utils, 'isValidEstateTrade').mockReturnValueOnce(true)
+      jest.spyOn(utils, 'isValidEstateTrade').mockResolvedValueOnce(true)
       mockPgQuery = jest.fn()
       ;(mockPg.withTransaction as jest.Mock).mockImplementation((fn, _onError) => fn({ query: mockPgQuery }))
 

test/unit/trades-logic.spec.ts

@@ -1,11 +1,24 @@
 /* eslint-disable @typescript-eslint/naming-convention */
-import { HDNodeWallet, TypedDataDomain, Wallet, zeroPadValue, toBeArray } from 'ethers'
+import { HDNodeWallet, TypedDataDomain, Wallet, zeroPadValue, toBeArray, Contract } from 'ethers'
 import { ChainId, Network, TradeAssetType, TradeCreation, TradeType } from '@dcl/schemas'
 import { ContractData, ContractName, getContract } from 'decentraland-transactions'
 import { fromMillisecondsToSeconds } from '../../src/logic/date'
-import { MARKETPLACE_TRADE_TYPES, getValueFromTradeAsset, validateTradeSignature } from '../../src/logic/trades/utils'
+import {
+  MARKETPLACE_TRADE_TYPES,
+  getValueFromTradeAsset,
+  isEstateFingerprintValid,
+  validateTradeSignature
+} from '../../src/logic/trades/utils'
 import { MarketplaceContractNotFound } from '../../src/ports/trades/errors'
 
+jest.mock('ethers', () => {
+  const originalModule = jest.requireActual('ethers')
+  return {
+    ...originalModule,
+    Contract: jest.fn()
+  }
+})
+
 describe('when verifying the trade signature', () => {
   let chainId: ChainId
   let trade: TradeCreation
@@ -139,3 +152,59 @@ describe('when verifying the trade signature', () => {
     })
   })
 })
+
+beforeEach(() => {
+  jest.resetAllMocks()
+})
+
+describe('when validating the estate signature', () => {
+  let contractAddress: string
+  let tokenId: string
+  let chainId: ChainId
+  let fingerprint: string
+
+  beforeEach(() => {
+    contractAddress = '0x9d32aac179153a991e832550d9f96441ea27763b'
+    tokenId = '5801'
+    chainId = ChainId.ETHEREUM_MAINNET
+    fingerprint = '0x1234567890'
+  })
+
+  describe('and the chain id is from a not supported chain', () => {
+    beforeEach(() => {
+      chainId = ChainId.AVALANCHE_MAINNET
+    })
+
+    it('should reject with an error', () => {
+      return expect(isEstateFingerprintValid(contractAddress, tokenId, chainId, fingerprint)).rejects.toThrow(Error)
+    })
+  })
+
+  describe('and the fingerprint is the same as the estate fingerprint', () => {
+    beforeEach(() => {
+      ;(Contract as jest.Mock).mockImplementationOnce(() => {
+        return {
+          getFingerprint: () => Promise.resolve(fingerprint)
+        }
+      })
+    })
+
+    it('should return true', () => {
+      return expect(isEstateFingerprintValid(contractAddress, tokenId, chainId, fingerprint)).resolves.toBe(true)
+    })
+  })
+
+  describe('and the fingerprint is different from the estate fingerprint', () => {
+    beforeEach(() => {
+      ;(Contract as jest.Mock).mockImplementationOnce(() => {
+        return {
+          getFingerprint: () => Promise.resolve('0x')
+        }
+      })
+    })
+
+    it('should return false', () => {
+      return expect(isEstateFingerprintValid(contractAddress, tokenId, chainId, fingerprint)).resolves.toBe(false)
+    })
+  })
+})

test/unit/trades-utils.spec.ts

@@ -17,6 +17,7 @@ import {
 } from '@dcl/schemas'
 import { getCategoryFromDBItem } from '../../src/adapters/items'
 import * as chainIdUtils from '../../src/logic/chainIds'
+import * as tradeLogicUtils from '../../src/logic/trades/utils'
 import { getItemByItemIdQuery } from '../../src/ports/items/queries'
 import { DBItem, ItemType } from '../../src/ports/items/types'
 import { getNftByTokenIdQuery } from '../../src/ports/nfts/queries'
@@ -277,12 +278,12 @@ describe("when validating the trade to see if it's a correct estate trade", () =
       trade.chainId = ChainId.AVALANCHE_MAINNET
     })
 
-    it('should throw the EstateContractNotFoundForChainId error', () => {
-      return expect(() => isValidEstateTrade(trade)).toThrow(new EstateContractNotFoundForChainId(ChainId.AVALANCHE_MAINNET))
+    it('should reject to throw the EstateContractNotFoundForChainId error', () => {
+      return expect(isValidEstateTrade(trade)).rejects.toThrow(new EstateContractNotFoundForChainId(ChainId.AVALANCHE_MAINNET))
     })
   })
 
-  describe('and the sent asset is an estate without fingerprint', () => {
+  describe("and there's a sent asset which is an estate without fingerprint", () => {
     beforeEach(() => {
       trade.sent = [
         {
@@ -295,11 +296,11 @@ describe("when validating the trade to see if it's a correct estate trade", () =
     })
 
     it('should return false', () => {
-      expect(isValidEstateTrade(trade)).toBe(false)
+      return expect(isValidEstateTrade(trade)).resolves.toBe(false)
     })
   })
 
-  describe('and the received asset is an estate without fingerprint', () => {
+  describe("and there's a received asset is an estate without fingerprint", () => {
     beforeEach(() => {
       trade.received = [
         {
@@ -313,7 +314,7 @@ describe("when validating the trade to see if it's a correct estate trade", () =
     })
 
     it('should return false', () => {
-      expect(isValidEstateTrade(trade)).toBe(false)
+      return expect(isValidEstateTrade(trade)).resolves.toBe(false)
     })
   })
 
@@ -329,8 +330,24 @@ describe("when validating the trade to see if it's a correct estate trade", () =
       ]
     })
 
-    it('should return true', () => {
-      expect(isValidEstateTrade(trade)).toBe(true)
+    describe('and the fingerprint is equal than the one in the contract', () => {
+      beforeEach(() => {
+        jest.spyOn(tradeLogicUtils, 'isEstateFingerprintValid').mockResolvedValue(true)
+      })
+
+      it('should return true', () => {
+        return expect(isValidEstateTrade(trade)).resolves.toBe(true)
+      })
+    })
+
+    describe('and the fingerprint is different than the one in the contract', () => {
+      beforeEach(() => {
+        jest.spyOn(tradeLogicUtils, 'isEstateFingerprintValid').mockResolvedValue(false)
+      })
+
+      it('should return false', () => {
+        return expect(isValidEstateTrade(trade)).resolves.toBe(false)
+      })
     })
   })
 
@@ -347,8 +364,24 @@ describe("when validating the trade to see if it's a correct estate trade", () =
       ]
     })
 
-    it('should return true', () => {
-      expect(isValidEstateTrade(trade)).toBe(true)
+    describe('and the fingerprint is equal than the one in the contract', () => {
+      beforeEach(() => {
+        jest.spyOn(tradeLogicUtils, 'isEstateFingerprintValid').mockResolvedValue(true)
+      })
+
+      it('should return true', () => {
+        return expect(isValidEstateTrade(trade)).resolves.toBe(true)
+      })
+    })
+
+    describe('and the fingerprint is different than the one in the contract', () => {
+      beforeEach(() => {
+        jest.spyOn(tradeLogicUtils, 'isEstateFingerprintValid').mockResolvedValue(false)
+      })
+
+      it('should return false', () => {
+        return expect(isValidEstateTrade(trade)).resolves.toBe(false)
+      })
     })
   })
 })