Canto Identity Subprotocols contest - adriro's results

Lines of code

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L144

Vulnerability details

Impact

The fuse function present in the Namespace contract mints a new Namespace NFT based on the given character data that references Tray tiles owned by the caller.

For each character, the implementation will use the characterToUnicodeBytes function from the Utils library to generate the corresponding unicode bytes for the given Tray tile data. The issue here is that the call hardcodes the _fontClass argument to 0 instead of using the fontClass attribute from the corresponding tile:

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L144

bytes memory charAsBytes = Utils.characterToUnicodeBytes(0, tileData.characterIndex, characterModifier);

Proof of Concept

As the call to characterToUnicodeBytes hardcodes the _fontClass argument to 0, then any minted tile with a value of fontClass different than 0 (this value ranges from 0 to 9) will be incorrectly generated.

1. User buys a Tray NFT such that one of its tiles has fontClass != 0.
2. User calls fuse in the Namespace contract passing the Tray token ID and tile offset from the previous step.
3. The fuse function will incorrectly generate the character using a value of fontClass == 0, the Namespace NFT will be minted with an incorrect associated name.

Recommendation

The function should call the characterToUnicodeBytes passing the correct value tileData.fontClass in the first argument:

bytes memory charAsBytes = Utils.characterToUnicodeBytes(tileData.fontClass, tileData.characterIndex, characterModifier);

Lines of code

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L103-L116

Vulnerability details

Impact

The tokenURI function present in the Bio contract copies the biography text associated to the NFT into the "description" field while rendering the json:

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L103-L116

string memory json = Base64.encode(
    bytes(
        string.concat(
            '{"name": "Bio #',
            LibString.toString(_id),
            '", "description": "',
            bioText,
            '", "image": "data:image/svg+xml;base64,',
            Base64.encode(bytes(string.concat(svg, text, "</text></svg>"))),
            '"}'
        )
    )
);
return string(abi.encodePacked("data:application/json;base64,", json));

This is susceptible to injection as the biography text is an input supplied by the user to the minting function. A bad actor can craft a biography text to escape from the description field and insert any arbitrary data after that.

Proof of concept

In the following test we insert an escaped double quote to break out of the description attribute, and inject a new attribute named "attribute" with a custom payload.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_Bio_tokenURI_Injection() public {
    string memory text = "description\",\"attribute\":\"injected arbitrary data";
    bio.mint(text);
    console.log(bio.tokenURI(1));
}

The resulting JSON has the new injected attribute:

{
  "name": "Bio #1", 
  "description": "description",
  "attribute":"injected arbitrary data", 
  "image": "data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHByZXNlcnZlQXNwZWN0UmF0aW89InhNaW5ZTWluIG1lZXQiIHZpZXdCb3g9IjAgMCA0MDAgMTAwIj48c3R5bGU+dGV4dCB7IGZvbnQtZmFtaWx5OiBzYW5zLXNlcmlmOyBmb250LXNpemU6IDEycHg7IH08L3N0eWxlPjx0ZXh0IHg9IjUwJSIgeT0iNTAlIiBkb21pbmFudC1iYXNlbGluZT0ibWlkZGxlIiB0ZXh0LWFuY2hvcj0ibWlkZGxlIj48dHNwYW4geD0iNTAlIiBkeT0iMjAiPmRlc2NyaXB0aW9uIiwiYXR0cmlidXRlIjoiaW5qZWN0ZWQgYXJiaXQ8L3RzcGFuPjx0c3BhbiB4PSI1MCUiIGR5PSIyMCI+cmFyeSBkYXRhPC90c3Bhbj48L3RleHQ+PC9zdmc+"
}

Recommendation

The tokenURI function should properly escape any user input, or alternatively forbid those special characters when minting the biography NFT.

Lines of code

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

Vulnerability details

Impact

The Bio tokenURI function splits biography text into lines. Since the Bio contract supports unicode, the algorithm tries to avoid splitting the line in between bytes that represent a single unicode character.

However, the implementation fails for certain unicode characters and will insert a line break in the middle of a single character, which changes the character into others before or after the line break. See PoC for a better understanding of the issue.

Proof of Concept

In the following test, we use a series of the "👁️‍🗨️" character as the biography text.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_Bio_tokenURI_BadLineBreak1() public {
    // The following text will be badly splitted
    string memory text = unicode"👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️👁️‍🗨️";
    bio.mint(string(text));
    console.log(bio.tokenURI(1));
}

As we can see in the resulting SVG, the algorithm splitted lines in between the character, which changes it into other unicode characters:

<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">👁️‍🗨️👁️‍🗨️👁️‍🗨</tspan><tspan x="50%" dy="20">️👁️‍🗨️👁️‍🗨</tspan><tspan x="50%" dy="20">️👁️‍🗨️👁️‍🗨️👁</tspan><tspan x="50%" dy="20">️‍🗨️👁️‍🗨️👁️‍🗨</tspan><tspan x="50%" dy="20">️👁️‍🗨️</tspan></text></svg>
``

In this other test, we use the england flag character "🏴󠁧󠁢󠁥󠁮󠁧󠁿":

```solidity
function test_Bio_tokenURI_BadLineBreak2() public {
    // The following text will be badly splitted
    string memory text = unicode"🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿";
    bio.mint(string(text));
    console.log(bio.tokenURI(1));
}

Again we see how the character is changed in between line breaks in the SVG:

<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">🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢</tspan><tspan x="50%" dy="20">󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧</tspan><tspan x="50%" dy="20">󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿🏴󠁧</tspan><tspan x="50%" dy="20">󠁢󠁥󠁮󠁧󠁿🏴󠁧󠁢󠁥󠁮</tspan><tspan x="50%" dy="20">󠁧󠁿🏴󠁧󠁢󠁥󠁮󠁧󠁿</tspan></text></svg>

Recommendation

The tokenURI function should implement proper unicode support if its intention is to manually handle line breaks. As this is a cumbersome task, the recommendation is to avoid splitting the text (or delegate it to the SVG in case such a feature exists).

Lines of code

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L162-L163

Vulnerability details

Impact

The Tray contract uses a pseudo-random algorithm to generate the tile data when a user buys and mints a new NFT. This is first defined by a seed which is the keccak256 hash of the last seed used:

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L162-L163

lastHash = keccak256(abi.encode(lastHash));
trayTiledata[j] = _drawing(uint256(lastHash));

The _drawing function uses this seed to generate a random value by calling the Utils.iteratePRNG function. This random value is then used to calculate the tile data based on range of 0 to SUM_ODDS values:

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L245-L273

function _drawing(uint256 _seed) private pure returns (TileData memory tileData) {
    uint256 res = _seed % SUM_ODDS;
    uint256 charRandValue = Utils.iteratePRNG(_seed); // Iterate PRNG to not have any biasedness / correlation between random numbers
    if (res < 32) {
        // Class is 0 in that case
        tileData.characterIndex = uint16(charRandValue % NUM_CHARS_EMOJIS);
    } else {
        tileData.characterIndex = uint16(charRandValue % NUM_CHARS_LETTERS);
        if (res < 64) {
            tileData.fontClass = 1;
            tileData.characterIndex = uint16(charRandValue % NUM_CHARS_LETTERS_NUMBERS);
        } else if (res < 80) {
            tileData.fontClass = 2;
        } else if (res < 96) {
            tileData.fontClass = 3 + uint8((res - 80) / 8);
        } else if (res < 104) {
            tileData.fontClass = 5 + uint8((res - 96) / 4);
        } else if (res < 108) {
            tileData.fontClass = 7 + uint8((res - 104) / 2);
            if (tileData.fontClass == 7) {
                // Set seed for Zalgo to ensure same characters will be always generated for this tile
                uint256 zalgoSeed = Utils.iteratePRNG(_seed);
                tileData.characterModifier = uint8(zalgoSeed % 256);
            }
        } else {
            tileData.fontClass = 9;
        }
    }
}

As this data is stored completely on-chain, any user can anticipate what tiles they will get when buying a Tray NFT:

1. Get the current value of lastHash.
2. Apply the keccak256 function to lastHash to get the next seed.
3. Replicate the logic present in the _drawing to calculate beforehand what NFT they will get.

While others mint NFTs, the buyer can then wait until the conditions are in place to mint the NFT they desire or mint a rare or more valuable NFT.

Note that this also allows the buyer to anticipate the data of any number of tiles, not just the next one. If the proximity is enough to justify the costs, the buyer can also buy NFTs in bulk (the Tray contract is an ERC721A) to ensure the NFT they want.

Proof of Concept

In this test we show how a buyer can easily anticipate the data for the next tile to be minted.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_Tray_buy_AnticipateNFTData() public {
    tray.endPrelaunchPhase();

    vm.startPrank(buyer);

    bytes32 currentHash = tray.lastHash();
    bytes32 nextHash = keccak256(abi.encode(currentHash));
    Tray.TileData memory predictedTileData = simulateDrawing(uint256(nextHash));

    tray.buy(1);

    Tray.TileData memory tileData = tray.getTile(1, 0);

    assertEq(tileData.fontClass, predictedTileData.fontClass);
    assertEq(tileData.characterIndex, predictedTileData.characterIndex);
    assertEq(tileData.characterModifier, predictedTileData.characterModifier);

    vm.stopPrank();
}

function simulateDrawing(uint256 _seed) internal pure returns (Tray.TileData memory tileData) {
    uint256 res = _seed % SUM_ODDS;
    uint256 charRandValue = Utils.iteratePRNG(_seed); // Iterate PRNG to not have any biasedness / correlation between random numbers
    if (res < 32) {
        // Class is 0 in that case
        tileData.characterIndex = uint16(charRandValue % NUM_CHARS_EMOJIS);
    } else {
        tileData.characterIndex = uint16(charRandValue % NUM_CHARS_LETTERS);
        if (res < 64) {
            tileData.fontClass = 1;
            tileData.characterIndex = uint16(charRandValue % NUM_CHARS_LETTERS_NUMBERS);
        } else if (res < 80) {
            tileData.fontClass = 2;
        } else if (res < 96) {
            tileData.fontClass = 3 + uint8((res - 80) / 8);
        } else if (res < 104) {
            tileData.fontClass = 5 + uint8((res - 96) / 4);
        } else if (res < 108) {
            tileData.fontClass = 7 + uint8((res - 104) / 2);
            if (tileData.fontClass == 7) {
                // Set seed for Zalgo to ensure same characters will be always generated for this tile
                uint256 zalgoSeed = Utils.iteratePRNG(_seed);
                tileData.characterModifier = uint8(zalgoSeed % 256);
            }
        } else {
            tileData.fontClass = 9;
        }
    }
}

Recommendation

The Tray contract shouldn't rely on a pseudo-random algorithm to generate tile data as this can be easily calculated beforehand. Consider using a VRF or an external oracle that provides stronger random guarantees.

Lines of code

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L60-L95

Vulnerability details

Impact

The Bio tokenURI function splits biography text into lines. The algorithm will take into account certain "continuation" characters to prevent splitting the line in the middle of these characters and keep accumulating those in the current line buffer (bytesLines):

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L60-L95

if ((i > 0 && (i + 1) % 40 == 0) || prevByteWasContinuation || i == lengthInBytes - 1) {
    bytes1 nextCharacter;
    if (i != lengthInBytes - 1) {
        nextCharacter = bioTextBytes[i + 1];
    }
    if (nextCharacter & 0xC0 == 0x80) {
        // Unicode continuation byte, top two bits are 10
        prevByteWasContinuation = true;
    } else {
        // Do not split when the prev. or next character is a zero width joiner. Otherwise, 👨‍👧‍👦 could become 👨>‍👧‍👦
        // Furthermore, do not split when next character is skin tone modifier to avoid 🤦‍♂️\n🏻
        if (
            // Note that we do not need to check i < lengthInBytes - 4, because we assume that it's a valid UTF8 string and these prefixes imply that another byte follows
            (nextCharacter == 0xE2 && bioTextBytes[i + 2] == 0x80 && bioTextBytes[i + 3] == 0x8D) ||
            (nextCharacter == 0xF0 &&
                bioTextBytes[i + 2] == 0x9F &&
                bioTextBytes[i + 3] == 0x8F &&
                uint8(bioTextBytes[i + 4]) >= 187 &&
                uint8(bioTextBytes[i + 4]) <= 191) ||
            (i >= 2 &&
                bioTextBytes[i - 2] == 0xE2 &&
                bioTextBytes[i - 1] == 0x80 &&
                bioTextBytes[i] == 0x8D)
        ) {
            prevByteWasContinuation = true;
            continue;
        }
        assembly {
            mstore(bytesLines, bytesOffset)
        }
        strLines[insertedLines++] = string(bytesLines);
        bytesLines = new bytes(80);
        prevByteWasContinuation = false;
        bytesOffset = 0;
    }
}

However, if the unicode string is a sequence of these continuation characters (which is a valid UTF8 string) the line buffer (which is 80 bytes) will eventually overflow and will revert the transaction due to an index of out bounds exception.

Proof of Concept

In the following test we use a string with 21 U+1F3FE characters to overflow the line buffer and revert the query to tokenURI.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_Bio_tokenURI_LineBufferOverflow() public {
    // This is a skin tone codepoint ("f09f8fbe") repeated 21 times
    string memory text = unicode"🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾🏾";
    bio.mint(string(text));
    vm.expectRevert();
    bio.tokenURI(1);
}

Recommendation

Change to a new line when the current line buffer is full.

Report

• Non Critical Issues (4)
• Low Issues (3)

Non Critical Issues

<a name="NC-1"></a>[NC-1] Import declarations should import specific symbols

Prefer import declarations that specify the symbol(s) using the form import {SYMBOL} from "SomeContract.sol" rather than importing the whole file

Instances (10):

File: canto-bio-protocol/src/Bio.sol

7: import "../interface/Turnstile.sol";

8: import "forge-std/Test.sol";

File: canto-namespace-protocol/src/Namespace.sol

7: import "./Tray.sol";

8: import "./Utils.sol";

9: import "../interface/Turnstile.sol";

File: canto-namespace-protocol/src/Tray.sol

10: import "./Utils.sol";

11: import "../interface/Turnstile.sol";

File: canto-namespace-protocol/src/Utils.sol

4: import "./Tray.sol";

File: canto-pfp-protocol/src/ProfilePicture.sol

5: import "../interface/Turnstile.sol";

6: import "../interface/ICidNFT.sol";

<a name="NC-2"></a>[NC-2] Use named parameters for mapping type declarations

Consider using named parameters in mappings (e.g. mapping(address account => uint256 balance)) to improve readability. This feature is present since Solidity 0.8.18

Instances (6):

File: canto-bio-protocol/src/Bio.sol

19:     mapping(uint256 => string) public bio;

File: canto-namespace-protocol/src/Namespace.sol

43:     mapping(string => uint256) public nameToToken;

46:     mapping(uint256 => string) public tokenToName;

49:     mapping(uint256 => Tray.TileData[]) private nftCharacters;

File: canto-namespace-protocol/src/Tray.sol

67:     mapping(uint256 => TileData[TILES_PER_TRAY]) private tiles;

File: canto-pfp-protocol/src/ProfilePicture.sol

32:     mapping(uint256 => ProfilePictureData) private pfp;

<a name="NC-3"></a>[NC-3] Use uint256 instead of the uint alias

Prefer using the uint256 type definition over its uint alias.

Instances (5):

File: canto-bio-protocol/src/Bio.sol

48:         uint lengthInBytes = bioTextBytes.length;

50:         uint lines = (lengthInBytes - 1) / 40 + 1;

56:         uint bytesOffset;

57:         for (uint i; i < lengthInBytes; ++i) {

115:             text = string.concat(text, '<tspan x="50%" dy="20">', strLines[i], "</tspan>");

<a name="NC-4"></a>[NC-4] Use constants for literal config values

Instances (27):

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L33
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L35
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L49
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L54
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L60
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L91
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-bio-protocol/src/Bio.sol#L123
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L81
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L83
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L112
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L117
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L140
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L110
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L112
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L122
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L152
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L226
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L238
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L248
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L253
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L256
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L258
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L260
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L262
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L267
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L60
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-pfp-protocol/src/ProfilePicture.sol#L62

Low Issues

<a name="L-1"></a>[L-1] Contract files should define a locked compiler version

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Instances (4):

File: canto-bio-protocol/src/Bio.sol

2: pragma solidity >=0.8.0;

File: canto-namespace-protocol/src/Namespace.sol

2: pragma solidity >=0.8.0;

File: canto-namespace-protocol/src/Tray.sol

2: pragma solidity >=0.8.0;

File: canto-pfp-protocol/src/ProfilePicture.sol

2: pragma solidity >=0.8.0;

<a name="L-2"></a>[L-2] Validate revenue addresses are not address(0)

Check that configured addresses are not address(0), else fees will be lost.

Instances (4):

• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L80
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L206
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L107
• https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Tray.sol#L220

<a name="L-3"></a>[L-3] Namespace burn should delete nftCharacters variable

https://github.com/code-423n4/2023-03-canto-identity/blob/main/canto-namespace-protocol/src/Namespace.sol#L184-L192

The burn function present in the Namespace contract should also delete and clear the associated data in the nftCharacters variable.

function burn(uint256 _id) external {
    address nftOwner = ownerOf(_id);
    if (nftOwner != msg.sender && getApproved[_id] != msg.sender && !isApprovedForAll[nftOwner][msg.sender])
        revert CallerNotAllowedToBurn();
    string memory associatedName = tokenToName[_id];
    delete tokenToName[_id];
    delete nameToToken[associatedName];
    delete nftCharacters[_id];
    _burn(_id);
}