Nft bridge

[critesjosh]

Description

This PR includes an AddressRegistry contract and a basic NftVault contract

Checklist:

• I have reviewed my diff in github, line by line.
• Every change is related to the PR description.
• There are no unexpected formatting changes, or superfluous debug logs.
• The branch has been rebased against the head of its merge target.
• I'm happy for the PR to be merged at the reviewers next convenience.
• NatSpec documentation of all the non-test functions is present and is complete.
• Continuous integration (CI) passes.
• Command forge coverage --match-contract MyContract returns 100% line coverage.
• All the possible reverts are tested.

[critesjosh]

@LHerskind I added functionality to the NftVault contract to enable transfers between vaults. My thinking is that vaults may have different functionality (buying vs selling vs governance, etc) so being able to transfer nfts between vaults directly from an aztec account would be useful.

[LHerskind]

It is not for no reason. The value is the amount of gas that is forwarded to the bridge. for the NFT case, you should look at what well-known collections spend when transferring as too little gas will make it revert and requires that the bridge is listed again with a higher gas limit.

[critesjosh]

Is this where we use the gas scripts to help estimate what the gas should be? For the NFTVaultGas.s.sol, the simulation runs but then fails when running the onchain simulation, not sure why.

I should also update the deployment scripts with these gas values, correct?

[LHerskind]

Yes. The output from the gas estimation is used here (with a bit to spare). I will take a look at measuring the gas to figure out the issue.

[critesjosh]

@LHerskind, @benesjan pointed out a griefing attack vector here. someone can register any NFT with any virtualAssetId. So if you get a virtual asset id with the intention of registering your NFT, i could just register any NFT in its place, so you can't deposit your NFT--that virtual asset id has been taken. There may be incentive to do this if we build a bridge for NFTs w/ governance rights, or something similar.

For a solution, we could have a depositor specify the collection address of the NFT that they want to deposit in step 1 of the deposit flow--they would pass the id (from the AddressRegistry) of the address of the collection in auxData, and during the deposit step the convert function would update the collection address in the corresponding nftAssets mapping. The NFTAsset struct would also include a isMatchPending flag, which would be set to true during the first step of the deposit flow as well.

Then in matchAndPull, there would be a check to make sure nftAssets[_virtualAssetId].collection == _collection and isMatchPending == true. Then the nftAssets mapping entry would be updated with the tokenId and set isMatchPending to false. This way the only "attack" would be for you to deposit an NFT from the same collection to my aztec account, costing me nothing.

[LHerskind]

Hmm, ye, good point. It makes the onboarding a bit more annoying, but ye for high-value cases you could be pretty annoying by frontrunning every time someone tries to matchAndPull 🤖. It will probably still be a vector for some of the NFTs where many "collections" live under the same address from the same brand. Can't remember the name, but believe one of the larger ones did so, artblocks if I recall correctly. But still much more limited.

The other attack still cost you something as you would lose gas money, but that should be small in comparison to the amount to grieve you for any non-valueless NFT.

[critesjosh]

Alternatives

1. we could require that the collection and the tokenId are passed in step 1 of the deposit, so it's not ambiguous which token the virtual asset is for. Then we wouldn't need the isMatchPending flag, since the virtual asset is only good for 1 token.

2. we could add another field to NFTAsset that specifies an address that can deposit NFTs to that virtual asset.

I think 1 might be the best solution

[LHerskind]

There is no isMatchPending in the NFTVault contract.

If you are using the auxData you would be able to fit most thing in, but some tokenIds would not fit.

I think going with sol 1 is probably a good way to handle it yes. If you update NFTAsset to be a struct of 3 elements, you can write the collection and tokenId directly and then it is just the flag for it being "deposited" that needs to be updated when matched? So storage wise should not be too different in costs. If we are anyway using the auxData such that not all can fit in, these 3 elements can also fit into one slot, so gas costs gets a bit lower with the restriction that addressable NFT space.

struct NFTAsset {
  address collection;
  uint48 tokenId;
  bool isInEscrow;
}

[critesjosh]

There is no isMatchPending in the NFTVault contract.

Right, I was just building off of the solution suggested in the first comment.

If we have users specify the collection and tokenId, do you think we need the isInEscrow flag?

With the restriction of specifying all of the token info in the first step of a deposit, a virtual asset id will always only be associated with 1 NFT. You'd be able to tell if the NFT was in escrow by looking up the owner of the NFT on the NFT contract. It might be a bit more convenient to do it with the isInEscrow flag in the NFTVault, but not sure if it is worth it.

[critesjosh]

also just thinking that if we pass the current NFT owner, collection and tokenId into the deposit flow, we can deposit the NFT into aztec in 1 transaction.

The owner will need to approve the bridge to do a transferFrom beforehand, but they could pass the owner to transferFrom as an id from the address registry as well.

This would be a better UX, but would be more restrictive in terms of input options.

owner and collection could each be 20 bits and offer ~1 million options each.
tokenId could be 24 bits and offer ~17 million options.

Do you foresee any problems with this approach? I think the UX improvements would be worth the input limitations

[LHerskind]

If the owner approves the NFT beforehand. Anyone can claim it. The contract should only ever pull NFTs or tokens from msg.sender, otherwise, you could use the rollup to steal the approved NFTs where the thief even will be hidden while doing it.