Skip to content

Commit

Permalink
fix: Prevent estate trades without fingerprints (#233)
Browse files Browse the repository at this point in the history
  • Loading branch information
LautaroPetaccio authored Dec 22, 2024
1 parent 6072e71 commit b76ad8b
Show file tree
Hide file tree
Showing 7 changed files with 232 additions and 24 deletions.
8 changes: 6 additions & 2 deletions src/controllers/handlers/trades-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import {
TradeEffectiveAfterExpirationError,
TradeNotFoundBySignatureError,
TradeNotFoundError,
DuplicateNFTOrderError
DuplicateNFTOrderError,
InvalidEstateTrade,
EstateContractNotFoundForChainId
} from '../../ports/trades/errors'
import { HTTPResponse, HandlerContextWithPath, StatusCode } from '../../types'

Expand Down Expand Up @@ -77,7 +79,9 @@ export async function addTradeHandler(
e instanceof InvalidTradeStructureError ||
e instanceof InvalidTradeSignatureError ||
e instanceof InvalidTradeSignerError ||
e instanceof InvalidECDSASignatureError
e instanceof InvalidECDSASignatureError ||
e instanceof InvalidEstateTrade ||
e instanceof EstateContractNotFoundForChainId
) {
return {
status: StatusCode.BAD_REQUEST,
Expand Down
9 changes: 7 additions & 2 deletions src/ports/trades/component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
TradeNotFoundError,
EventNotGeneratedError,
TradeNotFoundBySignatureError,
InvalidOwnerError
InvalidOwnerError,
InvalidEstateTrade
} from './errors'
import {
getInsertTradeAssetQuery,
Expand All @@ -23,7 +24,7 @@ import {
getTradeAssetsWithValuesByIdQuery
} from './queries'
import { DBTrade, DBTradeAsset, DBTradeAssetValue, DBTradeAssetWithValue, ITradesComponent, TradeEvent } from './types'
import { getNotificationEventForTrade, isERC721TradeAsset, validateTradeByType } from './utils'
import { getNotificationEventForTrade, isERC721TradeAsset, isEstateChain, isValidEstateTrade, validateTradeByType } from './utils'

export function createTradesComponent(components: Pick<AppComponents, 'dappsDatabase' | 'eventPublisher' | 'logs'>): ITradesComponent {
const { dappsDatabase: pg, eventPublisher, logs } = components
Expand Down Expand Up @@ -53,6 +54,10 @@ export function createTradesComponent(components: Pick<AppComponents, 'dappsData
if (!(await validateTradeByType(trade, pg))) {
throw new InvalidTradeStructureError(trade.type)
}
// Validate if estate trade is correct
if (isEstateChain(trade.chainId) && !isValidEstateTrade(trade)) {
throw new InvalidEstateTrade()
}

// vaidate signature
if (!validateTradeSignature(trade, signer)) {
Expand Down
12 changes: 12 additions & 0 deletions src/ports/trades/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ export class InvalidTradeStructureError extends Error {
}
}

export class EstateContractNotFoundForChainId extends Error {
constructor(public chainId: ChainId) {
super(`Estate contract not found for chainId ${chainId}`)
}
}

export class InvalidEstateTrade extends Error {
constructor() {
super("The Estate trade is invalid, check for the fingerprint to see if it matches the Estate's one")
}
}

export class InvalidTradeSignerError extends Error {
constructor() {
super('Trade and request signer do not match')
Expand Down
43 changes: 41 additions & 2 deletions src/ports/trades/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,23 @@ import {
ERC721TradeAsset,
CollectionItemTradeAsset,
ListingStatus,
Event
Event,
ChainId
} from '@dcl/schemas'
import { fromTradeAndAssetsToEventNotification } from '../../adapters/trades/trades'
import { getMarketplaceContracts } from '../../logic/contracts'
import { getBidsQuery } from '../bids/queries'
import { getItemByItemIdQuery, getItemsQuery } from '../items/queries'
import { DBItem } from '../items/types'
import { getNftByTokenIdQuery, getNFTsQuery } from '../nfts/queries'
import { DBNFT } from '../nfts/types'
import { DuplicatedBidError, DuplicateItemOrderError, DuplicateNFTOrderError, InvalidTradeStructureError } from './errors'
import {
DuplicatedBidError,
DuplicateItemOrderError,
DuplicateNFTOrderError,
EstateContractNotFoundForChainId,
InvalidTradeStructureError
} from './errors'
import { TradeEvent } from './types'

export function isERC20TradeAsset(asset: TradeAsset): asset is ERC20TradeAsset {
Expand All @@ -32,8 +40,39 @@ export function isCollectionItemTradeAsset(asset: TradeAsset): asset is Collecti
return asset.assetType === TradeAssetType.COLLECTION_ITEM
}

function isBytesEmpty(bytes: string): boolean {
return bytes === '0x' || bytes === ''
}

export function isEstateChain(chainId: ChainId): boolean {
return chainId !== ChainId.MATIC_AMOY && chainId !== ChainId.MATIC_MAINNET
}

export function isValidEstateTrade(trade: TradeCreation): 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
}

return true
}

export async function validateTradeByType(trade: TradeCreation, client: IPgComponent): Promise<boolean> {
const { sent, received, type } = trade

try {
if (type === TradeType.BID) {
// validate bid structure
Expand Down
17 changes: 17 additions & 0 deletions test/unit/trades-component.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
createTradesComponent
} from '../../src/ports/trades'
import {
InvalidEstateTrade,
InvalidTradeSignatureError,
InvalidTradeStructureError,
TradeAlreadyExpiredError,
Expand Down Expand Up @@ -142,6 +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)
})

it('should throw an InvalidTradeStructureError', async () => {
Expand All @@ -153,13 +155,27 @@ 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)
})

it('should throw an InvalidTradeSignatureError', async () => {
await expect(tradesComponent.addTrade(mockTrade, mockSigner)).rejects.toThrow(new InvalidTradeSignatureError())
})
})

describe('when a estate trade is not valid in the available estate chain ids', () => {
beforeEach(() => {
mockTrade.chainId = ChainId.ETHEREUM_SEPOLIA
jest.spyOn(signatureUtils, 'validateTradeSignature').mockReturnValue(true)
jest.spyOn(utils, 'validateTradeByType').mockResolvedValue(true)
jest.spyOn(utils, 'isValidEstateTrade').mockReturnValueOnce(false)
})

it('should throw an EstateTradeWithoutFingerprintError', async () => {
await expect(tradesComponent.addTrade(mockTrade, mockSigner)).rejects.toThrow(new InvalidEstateTrade())
})
})

describe('when the trade passes all validations', () => {
let mockPgQuery: jest.Mock
let insertedTrade: DBTrade
Expand All @@ -173,6 +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)
mockPgQuery = jest.fn()
;(mockPg.withTransaction as jest.Mock).mockImplementation((fn, _onError) => fn({ query: mockPgQuery }))

Expand Down
10 changes: 9 additions & 1 deletion test/unit/trades-handler.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import { Request } from 'node-fetch'
import { Event, Events, Trade, TradeCreation } from '@dcl/schemas'
import { ChainId, Event, Events, Trade, TradeCreation } from '@dcl/schemas'
import { addTradeHandler, getTradeAcceptedEventHandler, getTradeHandler } from '../../src/controllers/handlers/trades-handler'
import {
DuplicatedBidError,
EstateContractNotFoundForChainId,
InvalidEstateTrade,
EventNotGeneratedError,
InvalidTradeSignatureError,
InvalidTradeStructureError,
Expand Down Expand Up @@ -86,6 +88,12 @@ describe('when handling the creation of a new trade', () => {
{ errorName: 'TradeEffectiveAfterExpirationError', error: new TradeEffectiveAfterExpirationError(), code: StatusCode.BAD_REQUEST },
{ errorName: 'InvalidTradeStructureError', error: new InvalidTradeStructureError('bid'), code: StatusCode.BAD_REQUEST },
{ errorName: 'InvalidTradeSignatureError', error: new InvalidTradeSignatureError(), code: StatusCode.BAD_REQUEST },
{ errorName: 'EstateTradeWithoutFingerprintError', error: new InvalidEstateTrade(), code: StatusCode.BAD_REQUEST },
{
errorName: 'EstateContractNotFoundForChainId',
error: new EstateContractNotFoundForChainId(ChainId.AVALANCHE_MAINNET),
code: StatusCode.BAD_REQUEST
},
{ errorName: 'DuplicatedBidError', error: new DuplicatedBidError(), code: StatusCode.CONFLICT }
])('and the error is an instance of $errorName', ({ error, code }) => {
beforeEach(() => {
Expand Down
Loading

0 comments on commit b76ad8b

Please sign in to comment.