code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-canto-identity/blob/077372297fc419ea7688ab62cc3fd4e8f4e24e66/canto-bio-protocol/src/Bio.sol#L43

Vulnerability details

Impact

Difficulty to understand
The cost of the call
Terrible line breaks and word wrapping
Excessive complexity
no > UTF-8 support

Proof of Concept

The task of the tokenURI method is to return a json object with a generated svg where the biography up to 200 bytes long is divided by 40 per line. This was implemented by iterating through each character of the bio, with a large number of operations. Which makes it difficult to understand, increases the possibility of making mistakes, the cost of a method call, etc.

I recommend transferring the line break to the shoulders of SVG tags and styles, which will simplify and make the code many times cheaper, and get rid of potential errors.

An example is given below:

Old Flow

Code

Actual code code +-43 lines, 14 variables, a lot differens operations

Old SVG format

  <svg xmlns="http://www.w3.org/2000/svg" preserveAspectRatio="xMinYMin meet" viewBox="0 0 400 100">
    <style>
      text {
        font-family: sans-serif;
        font-size: 12px;
      }
    </style>
    <text x="50%" y="50%" dominant-baseline="middle" text-anchor="middle">
      <tspan x="50%" dy="20">Jane Doe, born in LA in 20, is an acclai</tspan>
      <tspan x="50%" dy="20">med actress director. She starred in t</tspan>
      <tspan x="50%" dy="20">he hit series 'City Light' and directed </tspan>
      <tspan x="50%" dy="20">the award-winning film 'Hidden Shadows'.</tspan>
      <tspan x="50%" dy="20"> Jane is also an animal rights activist.</tspan>
    </text>
  </svg>

Old SVG View (set additional 400px:200px):

New Flow:

SVG format

      <svg xmlns="http://www.w3.org/2000/svg" preserveAspectRatio="xMinYMin meet" viewBox="0 0 400 200">
        <style>
          .c {
            display: flex;
            align-items: center;
            justify-content: center;
            height: 100%;
          }
          .bio {
            font-family: sans-serif;
            font-size: 12px;
            max-width: 34ch;
            line-height: 20px;
            hyphens: auto;
          }
        </style>
        <foreignObject width="100%" height="100%">
          <div class="c" xmlns="http://www.w3.org/1999/xhtml">
            <div class="bio">Jane Doe, born in LA in 20, is an acclaimed actress director. She starred in the hit series 'City Light' and directed the award-winning film 'Hidden Shadows'. Jane is also an animal rights activist.</div>
          </div>
        </foreignObject>
      </svg>

New SVG view from example (set additional 400px:200px):

new svg view

This is just an example. The point is to use svg, css tags in order to reduce the complexity of the code many times

We can see that there is more plain text.

But now we don't have to worry about the following things:

Split rows
Support UTF-8
Correct transfer UTF-8 characters to new line etc
Avoiding potential problems
Most cheaper the gas
Complexity
etc..

Now we need to return only the template and insert the bio in the center of the template Code will look like:

    function tokenURI(uint256 _id) public view override returns (string memory) {
        if (_ownerOf[_id] == address(0)) revert TokenNotMinted(_id);
        string memory bioText = bio[_id];
        bytes memory imageBytes =  bytes(
            string.concat(
                '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 400 200"><style>.c{display:flex;align-items:center;justify-content:center;height:100%;}.bio{font-family:sans-serif;font-size:12px;max-width:34ch;line-height:20px;hyphens:auto;}</style><foreignObject width="100%" height="100%"><div class="c" xmlns="http://www.w3.org/1999/xhtml"><div class="bio">',
                bioText,
                "</div></div></foreignObject></svg>"
            )
        );
        string memory json = Base64.encode(
            bytes(
                string.concat(
                    '{"name": "Bio #',
                    LibString.toString(_id),
                    '", "description": "',
                    bioText,
                    '", "image": "data:image/svg+xml;base64,',
                    Base64.encode(imageBytes),
                    '"}'
                )
            )
        );
        return string.concat("data:application/json;base64,", json);
    }

We see a colossal simplification, With the avoidance of many moments

Of the problems is setting the styles correctly, because I just gave an example

Compare this with actual code code the method became cheaper by an order of magnitude, the complexity was reduced to a minimum. Now there are only 2 limitations for the biography, these are: length, and limitations to the XML text

Tools Used

Manual review

Recommended Mitigation Steps

Shift responsibility to SVG

c4-sponsor commented 1 year ago

OpenCoreCH marked the issue as sponsor confirmed

OpenCoreCH commented 1 year ago

Very good PoC, thanks for that! I think I'll ultimately go with the approach the warden suggested, as handling all edge cases properly on-chain is quite complex and error-prone.

0xleastwood commented 1 year ago

Very good PoC, thanks for that! I think I'll ultimately go with the approach the warden suggested, as handling all edge cases properly on-chain is quite complex and error-prone.

While I'd say this issue is extremely comprehensive, I don't really see any security vulnerability. These improvements are mostly cosmetic in nature so I think it makes sense to downgrade to QA.