c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L91 https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L117

Vulnerability details

Impact

According to the ERC1155 specification, The URI value allows for ID substitution by clients. If the string {id} exists in any URI, clients MUST replace this with the actual token ID in hexadecimal form.

There are two problems with the current codebase's implementation for URIs:

The metadata URI for each shareID is separate from the baseURI (the actual URI) path. This breaks the spec which mentions that clients MUST append the id to the baseURI (in our case id is the metadata URI of the shareID).
Even if it is a design choice, the metadata uri of a specific shareID does not use the ID substitution mechanism to retrieve the URI path of a token on-chain, which further breaks the spec.

In both these cases, there is no getURI() function implemented in Market.sol that enforces this ID substitution mechanism. Due to this, the ERC1155 metadata specification is not conformed to and thus broken.

Proof of Concept

Base URI being set in constructor:

File: Market.sol
91:     constructor(string memory _uri, address _paymentToken) ERC1155(_uri) Ownable() {

MetadataURI being set on Line 125 for shareID in createNewShare() function (we can see no appending is done here as well):

File: Market.sol
114:     function createNewShare(
115:         string memory _shareName,
116:         address _bondingCurve,
117:         string memory _metadataURI
118:     ) external onlyShareCreator returns (uint256 id) {
119:         require(whitelistedBondingCurves[_bondingCurve], "Bonding curve not whitelisted");
120:         require(shareIDs[_shareName] == 0, "Share already exists");
121:         id = ++shareCount; 
122:         shareIDs[_shareName] = id;
123:         shareData[id].bondingCurve = _bondingCurve; 
124:         shareData[id].creator = msg.sender;
125:         shareData[id].metadataURI = _metadataURI;
126:         emit ShareCreated(id, _shareName, _bondingCurve, msg.sender);
127:     }

Through the above two code blocks as well as no ID substitution mechanism implemented in the Market.sol contract, we can see how both the problems/cases mentioned above do not conform to the ERC1155 metadata specification.

Tools Used

Manual Review

Recommended Mitigation Steps

Solve either of the problem in the cases mentioned above to conform to the specification. This can be solved by appending the id to the base uri depending on the case being solved.

Case 1:

Append the metadata URI of shareID to baseURI when storing in the struct ShareData in createNewShare() function
Implement getURI() function which appends token numbers to the overall uri that was previously stored.

Case 2:

Implement getURI() function to append token numbers to metadata uri of shareID

Assessed type

Other

c4-pre-sort commented 10 months ago

minhquanym marked the issue as duplicate of #120

c4-judge commented 9 months ago

MarioPoneder changed the severity to QA (Quality Assurance)