Summary
When depositing an NFT, during calculating the base uri, if the collection returns empty string "", the protocol considers it as a valid base uri while the correct way is that it considers the token uri instead. This leads to a situation that wrong base uri and token uris are set for the NFT in the struct Request and forwarded to L2. On L2 side, the NFT will be minted (if it is not escrowed) without setting the base uri and token uri.
The root cause of the issue is that during calculating the base uri, returnValue is looking at the memory at the location 0x00 which is holding the offset of returned data, instead of looking at the location 0x20 where the length of returned data is located.
In other words, many NFTs have empty base uri as "", while they have non-empty uri for each token individually as token uri. Due to the explained issue, the protocol assumes that base uri and token uri for such NFTs are empty, while their token uri is not empty.
Vulnerability Details
During calculating the base uri in the function _callBaseUri, if the functions _baseUri() or baseUri() are public/external in the collection, then calling these functions will be successful.
function _callBaseUri(
address collection
)
internal
view
returns (bool, string memory)
{
bool success;
uint256 returnSize;
uint256 returnValue;
bytes memory ret;
bytes[2] memory encodedSignatures = [abi.encodeWithSignature("_baseUri()"), abi.encodeWithSignature("baseUri()")];
for (uint256 i = 0; i < 2; i++) {
bytes memory encodedParams = encodedSignatures[i];
assembly {
success := staticcall(gas(), collection, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
if success {
returnSize := returndatasize()
returnValue := mload(0x00)
ret := mload(0x40)
mstore(ret, returnSize)
returndatacopy(add(ret, 0x20), 0, returnSize)
mstore(0x40, add(add(ret, 0x20), add(returnSize, 0x20)))
}
}
if (success && returnSize >= 0x20 && returnValue > 0) {
return (true, abi.decode(ret, (string)));
}
}
return (false, "");
}
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/TokenUtil.sol#L139
If _baseUri() or baseUri() are public/external but their return string is "", it means that the base uri is not available, and their token uri should be used instead. For example, in the following collections, the function baseURi is public, but the state variable _baseURI is not set and is equal to "". In this case the function tokenUri should be used.
https://etherscan.io/address/0xbc0867d688bb604e4da8841066f3e72e5f17df3c#code
https://etherscan.io/address/0x3c3a2080ec9fecee7588838899f98dccf9d91868#readContract#F2
https://etherscan.io/address/0x1e31fe630faf5782f20a187d7d21958b5f3bd699#readContract#F2
function tokenURI(uint256 tokenId) public view virtual override returns (string memory) {
require(_exists(tokenId), "ERC721Metadata: URI query for nonexistent token");
string memory _tokenURI = _tokenURIs[tokenId];
string memory base = baseURI();
if (bytes(base).length == 0) {
return _tokenURI;
}
if (bytes(_tokenURI).length > 0) {
return string(abi.encodePacked(base, _tokenURI));
}
return string(abi.encodePacked(base, tokenId.toString()));
}
* @dev Returns the base URI set via {_setBaseURI}. This will be
* automatically added as a prefix in {tokenURI} to each token's URI, or
* to the token ID if no specific URI is set for that token ID.
*/
function baseURI() public view virtual returns (string memory) {
return _baseURI;
}
https://etherscan.io/address/0x16f6e045139011d825286c4569a599ee09274b98#code#F2#L142
When _baseUri() or baseUri on the target collection is called, the returned string data to the Ark protocol will be of format:
0x(32-byte offset)(32-byte length)(data)
for example, the returned string "a" will have the following format:
0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000016100000000000000000000000000000000000000000000000000000000000000
It shows that:
offset is: 0000000000000000000000000000000000000000000000000000000000000020
length is: 0000000000000000000000000000000000000000000000000000000000000001
data is: 6100000000000000000000000000000000000000000000000000000000000000
return size: 96 bytes
In this case, the variables success, returnSize, and returnValue would be: true, 96, and 32. So, the if-clause in the function _callBaseUri will be executed properly:
if (success && returnSize >= 0x20 && returnValue > 0) {
return (true, abi.decode(ret, (string)));
}
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/TokenUtil.sol#L166
The issue is here:
When _baseUri() or baseUri on the target collection is called, and they return the string "", the returned string data to the Ark protocol will be:
0x00000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000000
It shows that:
offset is: 0000000000000000000000000000000000000000000000000000000000000020
length is: 0000000000000000000000000000000000000000000000000000000000000000
return size: 64 bytes
In this case, the variables success, returnSize, and returnValue would be: true, 64, and 32. So, the if-clause in the function _callBaseUri will be executed. While it should not be executed, because the returned base uri is an empty string, and the token uri should be used instead.
The root cause of this issue is that returnValue (which is responsible to track the length of the returned string) is set to the memory at location 0x00, which is holding the offset of the returned data not the length of the returned string. The offset of the returned data for such a string is always 32. So, the condition returnValue > 0 is always satisfied which is not correct.
returnValue := mload(0x00)
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/TokenUtil.sol#L158
As a result of this issue the fields uri and tokenUris would be: "" and new string[](0)
(bool success, string memory _baseUri) = _callBaseUri(collection);
if (success) {
return (c.name(), c.symbol(), _baseUri, new string[](0));
}
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/token/TokenUtil.sol#L95
if (ctype == CollectionType.ERC721) {
(req.name, req.symbol, req.uri, req.tokenURIs) = TokenUtil.erc721Metadata(
collectionL1,
ids
);
}
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L121
On L2 side, during withdrawing, if the token is not escrowed, it would be minted. Since req.uris.len() is set to zero by mistake due to the bug explained, the body of the inner else-clause will be executed, while the body of inner if-clause should have been executed.
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
} else {
if (req.uris.len() != 0) {
let token_uri = req.uris[i];
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge_uri(to, token_id, token_uri.clone());
} else {
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge(to, token_id);
}
}
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L163
Then, the function mint_from_bridge would be called, instead of mint_from_bridge_uri, and as a result the uri for tokens will not be stored.
#[abi(embed_v0)]
impl ERC721BridgeableImpl of IERC721Bridgeable<ContractState> {
fn mint_from_bridge(ref self: ContractState, to: ContractAddress, token_id: u256) {
assert(
starknet::get_caller_address() == self.bridge.read(),
"ERC721: only bridge can mint"
);
self.erc721._mint(to, token_id);
}
fn mint_from_bridge_uri(ref self: ContractState, to: ContractAddress, token_id: u256, token_uri: ByteArray) {
IERC721Bridgeable::mint_from_bridge(ref self, to, token_id);
self.token_uris.write(token_id, token_uri);
}
}
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo#L80
In summary:
The NFT collections on L1 that do not have base uri but they have uri for each token individually, when they are bridged to L2, no base uri and no token uri will be set for them.
PoC
In the following test, the contract SampleNFT is deployed where _baseURI is not initialized, instead tokenURI should be used. When running the test test_skipping_token_uri, it is expected that the _tokenURIs[100] be returned as tokenUris[0], but it incorrectly returns "".
pragma solidity ^0.8.0;
import "forge-std/Test.sol";
import "../src/token/TokenUtil.sol";
import "forge-std/console.sol";
interface ISampleNFT {
function tokenURI(uint256 tokenId) external view returns (string memory);
function baseUri() external view returns (string memory);
}
contract SampleNFT is ISampleNFT {
mapping(uint256 => string) private _tokenURIs;
string private _baseURI;
string public name;
string public symbol;
constructor() {
_tokenURIs[100] = "https://ipfs...";
}
function tokenURI(uint256 tokenId) public view returns (string memory) {
string memory _tokenURI = _tokenURIs[tokenId];
string memory base = baseUri();
if (bytes(base).length == 0) {
return _tokenURI;
}
if (bytes(_tokenURI).length > 0) {
return string(abi.encodePacked(base, _tokenURI));
}
}
function baseUri() public view returns (string memory) {
return _baseURI;
}
function supportsInterface(
bytes4 interfaceId
) external view returns (bool) {
if (interfaceId == 0xffffffff) {
return false;
} else {
return true;
}
}
}
contract TokenUtilPoCTest is Test {
function test_skipping_token_uri() public {
address toCheck = address(new SampleNFT());
uint256[] memory tokenIds = new uint256[](1);
tokenIds[0] = 100;
(
string memory name,
string memory symbol,
string memory baseUri,
string[] memory tokenUris
) = TokenUtil.erc721Metadata(toCheck, tokenIds);
assert(
keccak256(abi.encodePacked(tokenUris[0])) ==
keccak256(abi.encodePacked(ISampleNFT(toCheck).tokenURI(100)))
);
}
}
Impact
Tools Used
Recommendations
The correct implementation is that 64 bytes from the returned data be stored in the memory from 0x00 to 0x40, and the return value be set to the memory at the location 0x20 where the length of the returned string is stored.
assembly {
- success := staticcall(gas(), collection, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
+ success := staticcall(gas(), collection, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x40)
if success {
returnSize := returndatasize()
- returnValue := mload(0x00)
+ returnValue := mload(0x20)
ret := mload(0x40)
mstore(ret, returnSize)
returndatacopy(add(ret, 0x20), 0, returnSize)
// update free memory pointer
mstore(0x40, add(add(ret, 0x20), add(returnSize, 0x20)))
}
}
if (success && returnSize >= 0x20 && returnValue > 0) {
return (true, abi.decode(ret, (string)));
}
Another solution is to check only the return data size to be larger than 64 bytes (offset + length = 64 byte):
assembly {
- success := staticcall(gas(), collection, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
+ success := staticcall(gas(), collection, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x00)
if success {
returnSize := returndatasize()
- returnValue := mload(0x00)
ret := mload(0x40)
mstore(ret, returnSize)
returndatacopy(add(ret, 0x20), 0, returnSize)
// update free memory pointer
mstore(0x40, add(add(ret, 0x20), add(returnSize, 0x20)))
}
}
- if (success && returnSize >= 0x20 && returnValue > 0) {
+ if (success && returnSize > 0x40) {
return (true, abi.decode(ret, (string)));
}
