Add on-chain validation to ERC20 wallet_watchAsset#1745
Conversation
|
In general for this task I was thinking that we would initially do validation in the confirmation itself as we do with That being said, I totally agree with your thoughts here - which mirror how I implemented the NFT part of the API:
but we need to get the greenlight from @rekmarks and @Gudahtt to change the expected behavior of this API (ignoring/deprecating detail params). |
|
Based on feedback from Dan, I've continued allowing |
| * @param address - ERC1155 asset contract address. | ||
| * @returns Promise resolving to whether the contract implements ERC1155 URI Metadata interface. | ||
| */ | ||
| contractSupportsURIMetadataInterface = async ( |
There was a problem hiding this comment.
Just curious was there any functional purpose to changing these from arrow functions?
There was a problem hiding this comment.
They were changed to make them mockable with jest. Don't expect any functional changes.
| interactingAddress: interactingAddress || this.config.selectedAddress, | ||
| }; | ||
|
|
||
| validateTokenToWatch(asset); |
There was a problem hiding this comment.
[nit] doesn't really matter but could have preserved this utility function and just passed in the provider do those operations.
There was a problem hiding this comment.
Yeah it could have. Just didn't feel like it gained much from being separate. Already tightly coupled to watchAsset, not more testable, and would need to go beyond validation by returning data to avoid duplicating logic.
| const stubCreateEthers = (ctrl: TokensController, res: () => boolean) => { | ||
| return sinon.stub(ctrl, '_createEthersContract').callsFake(() => { | ||
| return { | ||
| supportsInterface: sinon.stub().returns(res), | ||
| supportsInterface: sinon.stub().returns(res()), |
There was a problem hiding this comment.
?
const stubCreateEthers = (ctrl: TokensController, res: boolean) => {
return sinon.stub(ctrl, '_createEthersContract').callsFake(() => {
return {
supportsInterface: sinon.stub().resolves(res),
There was a problem hiding this comment.
Either returns or resolves seems to work. But I think res being a function is useful. The stub can be setup once in beforeEach with a default value, while allowing individual tests can overwrite the default. Otherwise every test would need to setup the stub.
adonesky1
left a comment
There was a problem hiding this comment.
A few nits but generally LGTM!
Explanation
Problem:
wallet_watchAssetacceptstype,address,symbol, anddecimalsfrom dapps. But it does not verify the accuracy of this information against the chain:type=ERC20, but provide anaddressthat is an EOA,ERC721, orERC1155contract.ERC20contract, but with the wrongsymbolordecimals. Malicious dapps can watch scam tokens under legit symbols, as seen here:Screen.Recording.2023-09-28.at.10.27.46.PM.mov
Solution:
Query the chain to get the contract type,
symbol, anddecimals. Validate the request against the on-chain data. Ifsymbolanddecimalsare defined in the contract (they're optional in ERC20), then the caller may now omit them in the request towallet_watchAsset. Otherwise they will be validated against the on-chain data.References
wallet_watchAssetconfirmation #1207Changelog
@metamask/assets-controllersTokensController.watchAssetnow performs on-chain validation ofthe asset's symbol and decimals, if they're defined in the contract. If the symbol and decimals are defined in the contract, they are no longer required to be passed to
watchAssetChecklist