looksrare/contracts-aggregator

LooksRare aggregator contracts

contracts-aggregator's Issues

0x Protocol integration

• Is your feature request related to a problem? Please describe.
  It could be interesting in a later phase to see if the aggregator model fits well with 0xProtocol (used by Coinbase) for NFT trading.

• Describe the solution you'd like
  Implement a proxy for 0x protocol.

• Additional context
  It would more for assessing the viability of the Order struct model with one additional marketplace.

Seaport v1.2

As Seaport is going to roll out v1.2, it is probably time to look at what can be done and how different it is to the current implementation (v1.1). 👌

Add support for test coverage

• Describe the bug
  It is currently not possible to measure the test coverage.

Readability for one-line statement

Statement (like below) are hard to read.

if (balance != 0) _executeERC20DirectTransfer(tokenTransfers[i].currency, recipient, balance);

This kind of 1-line statements make it much harder to read for things that can be state-changing. Since these don't impact the gas, I suggest rewriting them such as

if (balance != 0) {
   _executeERC20DirectTransfer(tokenTransfers[i].currency, recipient, balance);
}

The only exception can be the parts in ASM or the ones that trigger a reversion statement (so not state-changing).

Check return status

 assembly {
      if gt(selfbalance(), 1) {
          let status := call(gas(), originator, sub(selfbalance(), 1), 0, 0, 0, 0)
      }
  }

We need to check the call worked since the originator is not always the Erc20EnabledLooksRareAggregator contract.

https://github.com/LooksRare/contracts-aggregator/blob/master/contracts/LooksRareAggregator.sol#L99

Also, if we don't import it from the library, I'd like to remove the ones where the recipient is not the sender because they add to the (too) large size of the v2.

Replace low-level callers and import from contracts-libs

TBD once the NPM package is released.

Verify code coverage

For reference, LooksRare/contracts-exchange-v2#271

Re-entrancy tests

A scam NFT contract/recipient contract can steal user balance under the following scenarios

1. Scam NFT collections that re-enters LooksRareAggregator.execute during transferFrom
2. Scam recipient that re-enters LooksRareAggregator during onERC721Received

which then execute a non-atomic order that fails, and the aggregator returns the contract's ETH/ERC-20 balances to the scam contract which were originally provided by the user.

---

Original comment from Dingbatx:

Consider this :
1. User has 2 trades (A and B), each 1 ETH, and not atomic
2. User makes trade A and the one of the call (such as transferFrom) to the NFT contract is customized to reenter the aggregator contract's execute function.
3. Reentrancy function call makes at least 1 trade. At the end of execute, the remaining ETH and ERC20 balances (provided by the user) are refunded to the NFT contract.
4. Trade B will fail due to insufficient token/ETH balance but not revert due to the execution not being atomic.

In this case, the NFT contract that does the reentrancy can steal the balance of tokens/ETH in the aggregator contract during the time of reentrancy

Should indexing logic require actual Sweep events at the aggregator level?

Alternatives could be to use call traces or to use other marketplace events (e.g., Seaport, LR, Sudo) and build a logic using these sources of on-chain data.

The current SudoswapProxy implementation cannot handle partial fills when an NFT is no longer available

The current SudoswapProxy's partial fill can only handle the case where the pool price moves out of the user's max cost. It will revert during safeTransferFrom if the NFT the user wants to purchase is no longer available.

Sudoswap will release a V2 router and it will be able to handle this scenario by checking that the token IDs are still in the pool before order execution, so we will wait until it goes live before integrating with Sudoswap again.

LooksRare V2 Aggregation

Once protocol v2 is completed, it will be required to integrate it.

When refunding ERC-20 tokens to the user, should we leave 1 wei to save gas?

Rename the repo to contracts-aggregator

Add a single command to run all typescript tests

Currently,yarn test will fail.
Ideally, it should execute all tests under the respective "forked" environments at the blocks specified.
It should be renamed yarn test:hardhat too.

Numerator/denominator may need not to be equal to 1 for Seaport's advanced orders

• Is your feature request related to a problem? Please describe.
  Numerator/denominator values on the Seaport proxy are set at 1. It is possible that these values may need to vary from a fixed value of 1.

Update Solidity versions to match latest patched

Follow Foundry's best practices guide

https://book.getfoundry.sh/tutorials/best-practices

Add .env.template to the repo

Rescue ERC-721 and ERC-1155

In case an NFT is stuck in the aggregator

Add a security policy once it goes public

For reference, https://github.com/LooksRare/contracts-exchange-v1/security/policy