-
Notifications
You must be signed in to change notification settings - Fork 218
feat: enhance OFT token detection to block deposits #2537
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
Open
dewanshparashar
wants to merge
11
commits into
master
Choose a base branch
from
feat-enhance-oft-token-detection
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
57bd117
feat: enhance oft token detection to block deposits
dewanshparashar 2e24833
comment
dewanshparashar e0faf2d
dev: enhanced comment
dewanshparashar 29f1d93
comment
dewanshparashar f96b21c
dev: allow USDT transfers even if OFT
dewanshparashar eebc908
dev: add tests
dewanshparashar f9706ac
dev: add fallback detection of oftVersion check
dewanshparashar 830623e
dev: comms
dewanshparashar c765973
Merge branch 'master' of github.com:OffchainLabs/arbitrum-token-bridg…
dewanshparashar 0dc4026
Merge branch 'master' of github.com:OffchainLabs/arbitrum-token-bridg…
dewanshparashar 9d019f9
Merge branch 'master' into feat-enhance-oft-token-detection
brtkx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,13 @@ | |
|
||
import { ethers } from 'ethers' | ||
import { getProviderForChainId } from '@/token-bridge-sdk/utils' | ||
|
||
import axios from 'axios' | ||
import { isNetwork } from '../util/networks' | ||
import { ChainId } from '../types/ChainId' | ||
import { | ||
isTokenArbitrumOneUSDCe, | ||
isTokenArbitrumSepoliaUSDCe | ||
isTokenArbitrumSepoliaUSDCe, | ||
isTokenUSDT | ||
} from './TokenUtils' | ||
import { CommonAddress } from './CommonAddressUtils' | ||
|
||
|
@@ -281,29 +282,119 @@ export const withdrawOnlyTokens: { [chainId: number]: WithdrawOnlyToken[] } = { | |
] | ||
} | ||
|
||
async function isLayerZeroToken( | ||
/** | ||
* Fetches LayerZero's off-chain metadata (https://metadata.layerzero-api.com/v1/metadata) to identify OFT tokens. | ||
* If found in the metadata, it means the token supports OFT transfers - hence shouldn't be deposited through Arbitrum's canonical bridge. | ||
* @param parentChainErc20Address | ||
* @param parentChainId | ||
* @returns boolean - true if the token is an OFT token, false otherwise | ||
*/ | ||
async function isLayerZeroTokenViaAPI( | ||
parentChainErc20Address: string, | ||
parentChainId: number | ||
) { | ||
const parentProvider = getProviderForChainId(parentChainId) | ||
): Promise<boolean> { | ||
const chainIdToLzName: Record<number, string | undefined> = { | ||
[ChainId.Ethereum]: 'ethereum', | ||
[ChainId.ArbitrumOne]: 'arbitrum', | ||
[ChainId.ArbitrumNova]: 'nova', | ||
[ChainId.Sepolia]: 'ethereum-sepolia', | ||
[ChainId.ArbitrumSepolia]: 'arbitrum-sepolia' | ||
} | ||
|
||
const parentChainName = chainIdToLzName[parentChainId] | ||
|
||
if (!parentChainName) { | ||
// We can't check via the API if the chain is not supported | ||
throw new Error(`No LayerZero chain name for chain ID ${parentChainId}`) | ||
} | ||
|
||
// https://github.com/LayerZero-Labs/LayerZero-v2/blob/592625b9e5967643853476445ffe0e777360b906/packages/layerzero-v2/evm/oapp/contracts/oft/OFT.sol#L37 | ||
const layerZeroTokenOftContract = new ethers.Contract( | ||
parentChainErc20Address, | ||
[ | ||
'function oftVersion() external pure virtual returns (bytes4 interfaceId, uint64 version)' | ||
], | ||
parentProvider | ||
// Fetches LayerZero's off-chain metadata to identify OFT tokens. | ||
// If found in the metadata, it means the token supports OFT transfers - hence shouldn't be deposited through Arbitrum's canonical bridge. | ||
// Schema: | ||
// { | ||
// "ethereum": { <-- parent chain name | ||
// "tokens": { | ||
// "0x57e114b691db790c35207b2e685d4a43181e6061": { <--- parent chain erc20 address | ||
// "id": "ena", | ||
// "symbol": "ENA", | ||
// "decimals": 18 | ||
// } | ||
// ...more tokens | ||
// } | ||
// } | ||
// } | ||
|
||
const response = await axios.get( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason we use axios over fetch here? We don't seem to leverage any of the utilities of axios |
||
'https://metadata.layerzero-api.com/v1/metadata' | ||
) | ||
const metadata = response.data | ||
const chainData = metadata[parentChainName] | ||
|
||
if (chainData && chainData.tokens) { | ||
const tokenInfo = Object.keys(chainData.tokens).find( | ||
tokenAddr => | ||
tokenAddr.toLowerCase() === parentChainErc20Address.toLowerCase() | ||
) | ||
return !!tokenInfo | ||
} | ||
|
||
return false | ||
} | ||
|
||
/** | ||
* Checks if a token is an OFT token by querying the oftVersion function on the token contract. | ||
* @param parentChainErc20Address | ||
* @param parentChainId | ||
* @returns boolean - true if the token is an OFT token, false otherwise | ||
*/ | ||
async function isLayerZeroTokenOnChain( | ||
parentChainErc20Address: string, | ||
parentChainId: number | ||
): Promise<boolean> { | ||
try { | ||
const parentProvider = getProviderForChainId(parentChainId) | ||
// https://github.com/LayerZero-Labs/LayerZero-v2/blob/592625b9e5967643853476445ffe0e777360b906/packages/layerzero-v2/evm/oapp/contracts/oft/OFT.sol#L37 | ||
const layerZeroTokenOftContract = new ethers.Contract( | ||
parentChainErc20Address, | ||
[ | ||
'function oftVersion() external pure virtual returns (bytes4 interfaceId, uint64 version)' | ||
], | ||
parentProvider | ||
) | ||
const _isLayerZeroToken = await layerZeroTokenOftContract.oftVersion() | ||
return !!_isLayerZeroToken | ||
} catch (error) { | ||
// Assuming error means it's not an OFT token | ||
return false | ||
} | ||
} | ||
|
||
/** | ||
* Checks if a token is an OFT token by first trying the API check and then falling back to the on-chain check. | ||
* @param parentChainErc20Address | ||
* @param parentChainId | ||
* @returns boolean - true if the token is an OFT token, false otherwise | ||
*/ | ||
async function isLayerZeroToken( | ||
parentChainErc20Address: string, | ||
parentChainId: number | ||
) { | ||
try { | ||
if (await isLayerZeroTokenViaAPI(parentChainErc20Address, parentChainId)) { | ||
return true | ||
} | ||
} catch (e) { | ||
console.error( | ||
`Error checking LayerZero API for ${parentChainErc20Address} on chain ${parentChainId}. Falling back to on-chain check.`, | ||
e | ||
) | ||
// Fallback to on-chain check in case of API error | ||
return isLayerZeroTokenOnChain(parentChainErc20Address, parentChainId) | ||
} | ||
|
||
return false | ||
} | ||
|
||
/** | ||
* | ||
* @param erc20L1Address | ||
|
@@ -335,7 +426,10 @@ export async function isWithdrawOnlyToken({ | |
return true | ||
} | ||
|
||
if (await isLayerZeroToken(parentChainErc20Address, parentChainId)) { | ||
if ( | ||
!isTokenUSDT(parentChainErc20Address) && // USDT is a special case - it's bridged via OFT, but we still want to allow transfers coz of our OftV2 Integration | ||
(await isLayerZeroToken(parentChainErc20Address, parentChainId)) | ||
) { | ||
return true | ||
} | ||
|
||
|
90 changes: 90 additions & 0 deletions
90
packages/arb-token-bridge-ui/src/util/__tests__/WithdrawOnlyUtils.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
/** | ||
* @vitest-environment node | ||
*/ | ||
import { describe, it, expect } from 'vitest' | ||
import { isWithdrawOnlyToken } from '../WithdrawOnlyUtils' | ||
import { ChainId } from '../../types/ChainId' | ||
import { CommonAddress } from '../CommonAddressUtils' | ||
|
||
const networkTestTimeout = 10000 | ||
|
||
describe('isWithdrawOnlyToken', () => { | ||
const orbitChainId = 660279 // Xai | ||
|
||
it('should allow deposits for a standard token', async () => { | ||
const result = await isWithdrawOnlyToken({ | ||
parentChainErc20Address: '0x1234567890123456789012345678901234567890', | ||
parentChainId: ChainId.Ethereum, | ||
childChainId: ChainId.ArbitrumOne | ||
}) | ||
expect(result).toBe(false) | ||
}) | ||
|
||
it('should block deposits for a token in the withdraw-only list', async () => { | ||
const result = await isWithdrawOnlyToken({ | ||
parentChainErc20Address: '0x99D8a9C45b2ecA8864373A26D1459e3Dff1e17F3', // MIM on Arbitrum One | ||
parentChainId: ChainId.Ethereum, | ||
childChainId: ChainId.ArbitrumOne | ||
}) | ||
expect(result).toBe(true) | ||
}) | ||
|
||
it('should block deposits for USDC.e on an Orbit chain', async () => { | ||
const result = await isWithdrawOnlyToken({ | ||
parentChainErc20Address: CommonAddress.ArbitrumOne['USDC.e'], | ||
parentChainId: ChainId.ArbitrumOne, | ||
childChainId: orbitChainId | ||
}) | ||
expect(result).toBe(true) | ||
}) | ||
|
||
it('should allow deposits for USDC.e on a non-Orbit chain', async () => { | ||
const result = await isWithdrawOnlyToken({ | ||
parentChainErc20Address: CommonAddress.ArbitrumOne['USDC.e'], | ||
parentChainId: ChainId.Ethereum, | ||
childChainId: ChainId.ArbitrumOne | ||
}) | ||
expect(result).toBe(false) | ||
}) | ||
|
||
it( | ||
'should block deposits for a non-USDT LayerZero token', | ||
async () => { | ||
const result = await isWithdrawOnlyToken({ | ||
parentChainErc20Address: '0x57e114b691db790c35207b2e685d4a43181e6061', // ENA | ||
parentChainId: ChainId.Ethereum, | ||
childChainId: ChainId.ArbitrumOne | ||
}) | ||
expect(result).toBe(true) | ||
}, | ||
{ timeout: networkTestTimeout } | ||
) | ||
|
||
it( | ||
'should allow deposits for USDT as a special case', | ||
async () => { | ||
const usdtAddress = CommonAddress.Ethereum.USDT | ||
|
||
const result = await isWithdrawOnlyToken({ | ||
parentChainErc20Address: usdtAddress, | ||
parentChainId: ChainId.Ethereum, | ||
childChainId: ChainId.ArbitrumOne | ||
}) | ||
expect(result).toBe(false) | ||
}, | ||
{ timeout: networkTestTimeout } | ||
) | ||
|
||
it( | ||
'should allow deposits for a token not in LayerZero metadata', | ||
async () => { | ||
const result = await isWithdrawOnlyToken({ | ||
parentChainErc20Address: '0x9876543210987654321098765432109876543210', | ||
parentChainId: ChainId.Ethereum, | ||
childChainId: ChainId.ArbitrumOne | ||
}) | ||
expect(result).toBe(false) | ||
}, | ||
{ timeout: networkTestTimeout } | ||
) | ||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all values are defined, we can narrow down the type here