Skip to content
New issue

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

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

Already on GitHub? # to your account

Character limit for the "symbol" field #5398

Closed
mhhacker111 opened this issue Dec 23, 2024 · 2 comments
Closed

Character limit for the "symbol" field #5398

mhhacker111 opened this issue Dec 23, 2024 · 2 comments

Comments

@mhhacker111
Copy link

The "symbol" field, which is often accessible to any user of a smart contract when minting a new NFT, can be exploited for malicious purposes. A user could inject malicious JavaScript code via the "symbol" field, which might then be displayed in a web application, leading to an XSS attack—assuming the web application lacks output sanitization and a properly configured CSP (Content Security Policy).

In the field of cybersecurity, the principle of "Defense in Depth" applies, which emphasizes implementing mitigation mechanisms at every level of a system or application. Given that the "symbol" field is meant to represent only an abbreviated name for the NFT, it should not support an unlimited number of characters. Furthermore, careful consideration should be given to whether certain characters, such as < and >, should be permitted in this field.

Example of an attack:

https://solodit.cyfrin.io/issues/insufficient-input-validation-on-sablierv2nftdescriptorsafeassetsymbol-allows-an-attacker-to-obtain-stored-xss-codehawks-sablier-git

crStiv added a commit to crStiv/openzeppelin-contracts that referenced this issue Dec 25, 2024
Add maximum length check (11 characters)
   - Add validation for potentially dangerous characters
   - Fixes OpenZeppelin#5398
@Amxx
Copy link
Collaborator

Amxx commented Jan 3, 2025

ERC-721 describes the optional ERC721Metadata interface without providing any specifications about the symbol, other than it being a string.

It is important to note that the content of that string cannot attack the contract itself. Issues are on the front-ends that query these string (what is true of symbol also applies to name and tokenURI), and may not properly sanitize them before processing them.


IMO, any issue related to XSS (or similar) attack from an unsanitized string should be fixed on the the consumer side, not here! Other implementations of ERC-721 (and ERC-20, and ERC-1155) will not have this kind of checks, and front ends that consume these values will have to protect themselves no matter what we do here.

In the meantime, if someone wants to create a token with name = Love and symbol = <3, I don't think we should prevent that and push them to either using a different implementation (or modifiying this one).

@Amxx Amxx closed this as completed Jan 3, 2025
@mhhacker111
Copy link
Author

Your explanation regarding the lack of sanitization for certain characters is understandable. However, why not impose a limit on the number of characters for the symbol variable? Since it is merely a symbol, why allow the use of an unlimited number of characters?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants