Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nft bridge #291

Open
wants to merge 76 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
76 commits
Select commit Hold shift + click to select a range
5bdf8bf
address registry and nft vault
critesjosh Nov 30, 2022
f0296ef
updates
critesjosh Dec 1, 2022
596fe4a
e2e test
critesjosh Dec 1, 2022
4dc874c
update max int
critesjosh Dec 1, 2022
19f3ada
remove deposit param
critesjosh Dec 1, 2022
a370585
increase gas
critesjosh Dec 1, 2022
847cc31
e2e working
critesjosh Dec 1, 2022
52e1a43
add input value
critesjosh Dec 1, 2022
63ef608
add registry to constructor
critesjosh Dec 2, 2022
1aa9eb0
del
critesjosh Dec 2, 2022
869e81a
rename
critesjosh Dec 2, 2022
eafca9e
deposit + withdraw
critesjosh Dec 2, 2022
86e68f7
add 0 output check
critesjosh Dec 2, 2022
202c05e
update comments
critesjosh Dec 2, 2022
d0668a6
add tests
critesjosh Dec 2, 2022
4936461
del
critesjosh Dec 2, 2022
69463ee
cleanup comments
critesjosh Dec 2, 2022
271abda
Merge branch 'AztecProtocol:master' into nft-bridge
critesjosh Dec 2, 2022
c3b5ce0
fmt
critesjosh Dec 3, 2022
27c6167
Merge branch 'nft-bridge' of https://github.com/critesjosh/aztec-conn…
critesjosh Dec 3, 2022
0ab5406
add L1 option
critesjosh Dec 3, 2022
1288f97
update inc
critesjosh Dec 3, 2022
b845e70
remove comments, fix matchDeposit
critesjosh Dec 6, 2022
217a526
rename id, cleanup
critesjosh Dec 6, 2022
4355353
update tests
critesjosh Dec 6, 2022
6559576
check event
critesjosh Dec 6, 2022
cfe88ae
fmt
critesjosh Dec 6, 2022
a5d6705
clean up comments
critesjosh Dec 6, 2022
a10c6ba
clean up comments
critesjosh Dec 6, 2022
469c372
clean comments
critesjosh Dec 6, 2022
281be8f
clean comments
critesjosh Dec 6, 2022
5c2c0b3
add deposit/withdraw events
critesjosh Dec 8, 2022
ad48137
revert reason for invalid input/outputs
critesjosh Dec 8, 2022
3473dd6
deployments
critesjosh Dec 8, 2022
1ae3752
gas benchmarks
critesjosh Dec 8, 2022
aa7049e
test deposit/withdraw events
critesjosh Dec 8, 2022
746f595
fmt
critesjosh Dec 8, 2022
0e9ead3
Merge branch 'AztecProtocol:master' into nft-bridge
critesjosh Dec 8, 2022
055c399
natspec comments
critesjosh Dec 8, 2022
cdfacf9
Merge branch 'nft-bridge' of https://github.com/critesjosh/aztec-conn…
critesjosh Dec 8, 2022
41004f2
fmt
critesjosh Dec 8, 2022
623725e
add test
critesjosh Dec 8, 2022
55031c9
fix linting
critesjosh Dec 8, 2022
32214eb
fmt
critesjosh Dec 8, 2022
ae33651
uint256 addressCount
critesjosh Dec 8, 2022
a4dbf00
incorporate lasse's feedback
critesjosh Dec 8, 2022
39984bd
Merge remote-tracking branch 'upstream/master' into nft-bridge
critesjosh Dec 9, 2022
d0b3cef
fix test
critesjosh Dec 9, 2022
8367e39
fmt
critesjosh Dec 9, 2022
b6e18ea
add transfer functionality
critesjosh Dec 24, 2022
249a917
move asset defs to storage
critesjosh Dec 31, 2022
55f3064
add .env
critesjosh Dec 31, 2022
82f7617
edits
critesjosh Dec 31, 2022
7002556
add .env
critesjosh Dec 31, 2022
22588ab
add .env
critesjosh Dec 31, 2022
4e937c8
add readme
critesjosh Dec 31, 2022
7741364
update address id
critesjosh Dec 31, 2022
73124ef
add readme
critesjosh Dec 31, 2022
f6d8757
add content
critesjosh Dec 31, 2022
4f91cf8
update
critesjosh Dec 31, 2022
62f2f20
fmt, lint
critesjosh Dec 31, 2022
d65c41c
Merge branch 'nft-bridge' into nft-transfer-bridge
critesjosh Dec 31, 2022
625f166
Merge pull request #2 from critesjosh/nft-transfer-bridge
critesjosh Dec 31, 2022
e00d25a
Merge branch 'AztecProtocol:master' into nft-bridge
critesjosh Dec 31, 2022
e00d03d
Update .gitignore
critesjosh Jan 3, 2023
5420aed
fix hardcoded nonce
critesjosh Jan 3, 2023
586ef5d
Merge branch 'nft-bridge' of https://github.com/critesjosh/aztec-conn…
critesjosh Jan 3, 2023
9f7be07
remove unreachable code
critesjosh Jan 3, 2023
c50d702
test invalid input amount
critesjosh Jan 3, 2023
f9fd54e
incorporat e review suggestions
critesjosh Jan 4, 2023
e21dd63
Delete .env
critesjosh Jan 4, 2023
7feeec7
Merge branch 'AztecProtocol:master' into nft-bridge
critesjosh Jan 4, 2023
7cef40c
fmt
critesjosh Jan 4, 2023
54833bd
Merge branch 'AztecProtocol:master' into nft-bridge
critesjosh Jan 10, 2023
a4e70ca
fmt
critesjosh Jan 10, 2023
c5fd393
retry fmt
critesjosh Jan 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ out/
node_modules/
yarn-error.log
typechain-types/
broadcast/
broadcast/
.env
2 changes: 1 addition & 1 deletion lib/rollup-encoder
135 changes: 135 additions & 0 deletions src/bridges/nft-basic/NFTVault.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2022 Aztec.
pragma solidity >=0.8.4;

import {IERC721} from "../../../lib/openzeppelin-contracts/contracts/interfaces/IERC721.sol";
import {AztecTypes} from "../../../lib/rollup-encoder/src/libraries/AztecTypes.sol";
import {ErrorLib} from "../base/ErrorLib.sol";
import {BridgeBase} from "../base/BridgeBase.sol";
import {AddressRegistry} from "../registry/AddressRegistry.sol";

/**
* @title Basic NFT Vault for Aztec.
* @author Josh Crites, (@critesjosh on Github), Aztec Team
* @notice You can use this contract to hold your NFTs on Aztec. Whoever holds the corresponding virutal asset note can withdraw the NFT.
* @dev This bridge demonstrates basic functionality for an NFT bridge. This may be extended to support more features.
*/
contract NFTVault is BridgeBase {
struct NFTAsset {
address collection;
uint256 tokenId;
}

AddressRegistry public immutable REGISTRY;

mapping(uint256 => NFTAsset) public nftAssets;

error InvalidVirtualAssetId();

event NFTDeposit(uint256 indexed virtualAssetId, address indexed collection, uint256 indexed tokenId);
event NFTWithdraw(uint256 indexed virtualAssetId, address indexed collection, uint256 indexed tokenId);

/**
* @notice Set the addresses of RollupProcessor and AddressRegistry
* @param _rollupProcessor Address of the RollupProcessor
* @param _registry Address of the AddressRegistry
*/
constructor(address _rollupProcessor, address _registry) BridgeBase(_rollupProcessor) {
REGISTRY = AddressRegistry(_registry);
}

/**
* @notice Function for the first step of a NFT deposit, a NFT withdrawal, or transfer to another NFTVault.
* @dev This method can only be called from the RollupProcessor. The first step of the
* deposit flow returns a virutal asset note that will represent the NFT on Aztec. After the
* virutal asset note is received on Aztec, the user calls matchAndPull which deposits the NFT
* into Aztec and matches it with the virtual asset. When the virutal asset is sent to this function
* it is burned and the NFT is sent to the recipient passed in _auxData.
*
* @param _inputAssetA - ETH (Deposit) or VIRTUAL (Withdrawal)
* @param _outputAssetA - VIRTUAL (Deposit) or 0 ETH (Withdrawal)
* @param _totalInputValue - must be 1 wei (Deposit) or 1 VIRTUAL (Withdrawal)
* @param _interactionNonce - A globally unique identifier of this interaction/`convert(...)` call
* corresponding to the returned virtual asset id
* @param _auxData - corresponds to the Ethereum address id in the AddressRegistry.sol for withdrawals
* @return outputValueA - 1 VIRTUAL asset (Deposit) or 0 ETH (Withdrawal)
*
*/

function convert(
AztecTypes.AztecAsset calldata _inputAssetA,
AztecTypes.AztecAsset calldata,
AztecTypes.AztecAsset calldata _outputAssetA,
AztecTypes.AztecAsset calldata,
uint256 _totalInputValue,
uint256 _interactionNonce,
uint64 _auxData,
address
)
external
payable
override(BridgeBase)
onlyRollup
returns (uint256 outputValueA, uint256 outputValueB, bool isAsync)
{
if (
_inputAssetA.assetType == AztecTypes.AztecAssetType.NOT_USED
|| _inputAssetA.assetType == AztecTypes.AztecAssetType.ERC20
) revert ErrorLib.InvalidInputA();
if (
_outputAssetA.assetType == AztecTypes.AztecAssetType.NOT_USED
|| _outputAssetA.assetType == AztecTypes.AztecAssetType.ERC20
) revert ErrorLib.InvalidOutputA();
if (_totalInputValue != 1) {
revert ErrorLib.InvalidInputAmount();
}
if (
_inputAssetA.assetType == AztecTypes.AztecAssetType.ETH
&& _outputAssetA.assetType == AztecTypes.AztecAssetType.VIRTUAL
) {
return (1, 0, false);
} else if (_inputAssetA.assetType == AztecTypes.AztecAssetType.VIRTUAL) {
NFTAsset memory token = nftAssets[_inputAssetA.id];
if (token.collection == address(0x0)) {
revert ErrorLib.InvalidInputA();
}

address to = REGISTRY.addresses(_auxData);
if (to == address(0x0)) {
revert ErrorLib.InvalidAuxData();
}
delete nftAssets[_inputAssetA.id];
emit NFTWithdraw(_inputAssetA.id, token.collection, token.tokenId);

if (_outputAssetA.assetType == AztecTypes.AztecAssetType.ETH) {
IERC721(token.collection).transferFrom(address(this), to, token.tokenId);
return (0, 0, false);
} else {
IERC721(token.collection).approve(to, token.tokenId);
NFTVault(to).matchAndPull(_interactionNonce, token.collection, token.tokenId);
return (1, 0, false);
}
}
}

/**
* @notice Function for the second step of a NFT deposit or for transfers from other NFTVaults.
* @dev For a deposit, this method is called by an Ethereum L1 account that owns the NFT to deposit.
* The user must approve this bridge contract to transfer the users NFT before this function
* is called. This function assumes the NFT contract complies with the ERC721 standard.
* For a transfer from another NFTVault, this method is called by the NFTVault that is sending the NFT.
*
* @param _virtualAssetId - the virutal asset id of the note returned in the deposit step of the convert function
* @param _collection - collection address of the NFT
* @param _tokenId - the token id of the NFT
*/

function matchAndPull(uint256 _virtualAssetId, address _collection, uint256 _tokenId) external {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (nftAssets[_virtualAssetId].collection != address(0x0)) {
revert InvalidVirtualAssetId();
}
nftAssets[_virtualAssetId] = NFTAsset({collection: _collection, tokenId: _tokenId});
IERC721(_collection).transferFrom(msg.sender, address(this), _tokenId);
emit NFTDeposit(_virtualAssetId, _collection, _tokenId);
}
}
87 changes: 87 additions & 0 deletions src/bridges/registry/AddressRegistry.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2022 Aztec.
pragma solidity >=0.8.4;

import {AztecTypes} from "../../../lib/rollup-encoder/src/libraries/AztecTypes.sol";
import {ErrorLib} from "../base/ErrorLib.sol";
import {BridgeBase} from "../base/BridgeBase.sol";

/**
* @title Aztec Address Registry.
* @author Josh Crites (@critesjosh on Github), Aztec team
* @notice This contract can be used to anonymously register an ethereum address with an id.
* This is useful for reducing the amount of data required to pass an ethereum address through auxData.
* @dev Use this contract to lookup ethereum addresses by id.
*/
contract AddressRegistry is BridgeBase {
uint256 public addressCount;
mapping(uint256 => address) public addresses;

event AddressRegistered(uint256 indexed index, address indexed entity);

/**
* @notice Set address of rollup processor
* @param _rollupProcessor Address of rollup processor
*/
constructor(address _rollupProcessor) BridgeBase(_rollupProcessor) {}

/**
* @notice Function for getting VIRTUAL assets (step 1) to register an address and registering an address (step 2).
* @dev This method can only be called from the RollupProcessor. The first step to register an address is for a user to
* get the type(uint160).max value of VIRTUAL assets back from the bridge. The second step is for the user
* to send an amount of VIRTUAL assets back to the bridge. The amount that is sent back is equal to the number of the
* ethereum address that is being registered (e.g. uint160(0x2e782B05290A7fFfA137a81a2bad2446AD0DdFEB)).
*
* @param _inputAssetA - ETH (step 1) or VIRTUAL (step 2)
* @param _outputAssetA - VIRTUAL (steps 1 and 2)
* @param _totalInputValue - must be 1 wei (ETH) (step 1) or address value (step 2)
* @return outputValueA - type(uint160).max (step 1) or 0 VIRTUAL (step 2)
*
*/

function convert(
AztecTypes.AztecAsset calldata _inputAssetA,
AztecTypes.AztecAsset calldata,
AztecTypes.AztecAsset calldata _outputAssetA,
AztecTypes.AztecAsset calldata,
uint256 _totalInputValue,
uint256,
uint64,
address
) external payable override(BridgeBase) onlyRollup returns (uint256 outputValueA, uint256, bool) {
if (
_inputAssetA.assetType == AztecTypes.AztecAssetType.NOT_USED
|| _inputAssetA.assetType == AztecTypes.AztecAssetType.ERC20
) revert ErrorLib.InvalidInputA();
if (_outputAssetA.assetType != AztecTypes.AztecAssetType.VIRTUAL) {
revert ErrorLib.InvalidOutputA();
}
if (_inputAssetA.assetType == AztecTypes.AztecAssetType.ETH) {
if (_totalInputValue != 1) {
revert ErrorLib.InvalidInputAmount();
critesjosh marked this conversation as resolved.
Show resolved Hide resolved
}
return (type(uint160).max, 0, false);
} else if (_inputAssetA.assetType == AztecTypes.AztecAssetType.VIRTUAL) {
address toRegister = address(uint160(_totalInputValue));
registerAddress(toRegister);
return (0, 0, false);
}
}

/**
* @notice Register an address at the registry
* @dev This function can be called directly from another Ethereum account. This can be done in
* one step, in one transaction. Coming from Ethereum directly, this method is not as privacy
* preserving as registering an address through the bridge.
*
* @param _to - The address to register
* @return addressCount - the index of address that has been registered
*/

function registerAddress(address _to) public returns (uint256) {
uint256 userIndex = addressCount++;
addresses[userIndex] = _to;
emit AddressRegistered(userIndex, _to);
return userIndex;
}
}
42 changes: 42 additions & 0 deletions src/deployment/nft-basic/NFTVaultDeployment.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2022 Aztec.
pragma solidity >=0.8.4;

import {BaseDeployment} from "../base/BaseDeployment.s.sol";
import {NFTVault} from "../../bridges/nft-basic/NFTVault.sol";
import {AddressRegistry} from "../../bridges/registry/AddressRegistry.sol";

contract NFTVaultDeployment is BaseDeployment {
function deploy(address _addressRegistry) public returns (address) {
emit log("Deploying NFTVault bridge");

vm.broadcast();
NFTVault bridge = new NFTVault(ROLLUP_PROCESSOR, _addressRegistry);

emit log_named_address("NFTVault bridge deployed to", address(bridge));

return address(bridge);
}

function deployAndList(address _addressRegistry) public returns (address) {
address bridge = deploy(_addressRegistry);

uint256 addressId = listBridge(bridge, 135500);
emit log_named_uint("NFTVault bridge address id", addressId);

return bridge;
}

function deployAndListAddressRegistry() public returns (address) {
emit log("Deploying AddressRegistry bridge");

AddressRegistry bridge = new AddressRegistry(ROLLUP_PROCESSOR);

emit log_named_address("AddressRegistry bridge deployed to", address(bridge));

uint256 addressId = listBridge(address(bridge), 120500);
emit log_named_uint("AddressRegistry bridge address id", addressId);

return address(bridge);
}
}
28 changes: 28 additions & 0 deletions src/deployment/registry/AddressRegistryDeployment.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// SPDX-License-Identifier: Apache-2.0
// Copyright 2022 Aztec.
pragma solidity >=0.8.4;

import {BaseDeployment} from "../base/BaseDeployment.s.sol";
import {AddressRegistry} from "../../bridges/registry/AddressRegistry.sol";

contract AddressRegistryDeployment is BaseDeployment {
function deploy() public returns (address) {
emit log("Deploying AddressRegistry bridge");

vm.broadcast();
AddressRegistry bridge = new AddressRegistry(ROLLUP_PROCESSOR);

emit log_named_address("AddressRegistry bridge deployed to", address(bridge));

return address(bridge);
}

function deployAndList() public returns (address) {
address bridge = deploy();

uint256 addressId = listBridge(bridge, 120500);
emit log_named_uint("AddressRegistry bridge address id", addressId);

return bridge;
}
}
Loading