Summary
TokenUtil::_callBaseUri is used to get the baseURI of an NFT collection but it uses wrong function signatures, causing it to always return an empty string even if the collection has a baseURI set.
Vulnerability Details
Currently there are two ways for how an NFT's URI can be included in a request when bridging NFTs.
Setting a base URI (uri on L1, base_uri on L2)
Setting an array of token URIs (tokenURIs on L1, ids on L2)
If a collection has set a base URI, the bridge is supposed to take approach 1) as it is more efficient to add one URI from which individual token URIs can be derived instead of adding a large array of URIs to the request.
This also allows the bridging of more NFTs at once, as it is more size efficient.
The retrieving of the baseURI on L1 is handled by TokenUtil::_callBaseUri which is used by TokenUtil::erc721Metadata to retrieve an NFT's metadata.
function _callBaseUri(
address collection
)
internal
view
returns (bool, string memory)
{
bool success;
require(success == false);
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, "");
}
Looking at this function, at [1] it uses two hardcoded signatures, _baseUri() and baseUri(), which it calls via a staticcall to get a collection's baseURI.
Now the problem is, that these signatures are incorrect. This can be seen if we look at the OpenZeppelin documentation of ERC721Metadata (https://docs.openzeppelin.com/contracts/2.x/api/token/erc721#ERC721Metadata-baseURI--).
Here we can see that the function signature to get the baseURI of a ERC721Metadata compliant collection is baseURI() and not baseUri().
Impact
This discrepancy in function signatures between the standard and the implementation breaks the whole baseURI functionality of the bridge. More precisely it causes completely compliant NFT collections which in fact use a baseURI to be unable to use that feature of the bridge.
Additionally, this feature being broken decreases the amount of NFTs that can be bridged with one transaction as multiple tokenURIs create a larger request than one baseURI.
Proof of Concept
This PoC will show that the bridge does not use the baseURI of a ERC721Metadata compliant collection.
First please add the following files to apps/blockchain/ethereum/src/token/:
IERC721BridgeableMetadata.sol:
pragma solidity ^0.8.0;
interface IERC721BridgeableMetadata {
function initialize(bytes calldata data) external;
function setBaseURI(string calldata baseURI_) external;
function setTokenURI(uint256 tokenId, string calldata _tokenURI) external;
function mintWithTokenURI(address to, uint256 tokenId, string calldata _tokenURI) external;
function tokenURI(uint256 tokenId) external view returns (string memory);
}
ERC721BridgeableMetadata.sol:
pragma solidity ^0.8.0;
import "openzeppelin-contracts/contracts/token/ERC721/ERC721.sol";
import "./IERC721Bridgeable.sol";
import "../UUPSProxied.sol";
@title ERC721 that can be minted by the bridge.
@dev As this ERC721 must be upgradable, the name and symbol must
be overriden to work correctly, as the constructor can't be called,
but initialization function instead.
*/
contract ERC721BridgeableMetadata is ERC721, UUPSOwnableProxied, IERC721Bridgeable {
string private _name;
string private _symbol;
string private __baseURI;
mapping(uint256 => string) private _tokenURIs;
@notice Default constructor, but intialize is used instead.
*/
constructor()
ERC721("", "")
{ }
@notice Initializes the implementation, only callable once.
@param data Data to init the implementation.
*/
function initialize(
bytes calldata data
)
public
onlyInit
{
(string memory n, string memory s) = abi.decode(data, (string, string));
_name = n;
_symbol = s;
_transferOwnership(_msgSender());
}
@inheritdoc IERC721Bridgeable
@dev In this implementation, the owner is the bridge by default. So `onlyOwner`
is enough.
*/
function mintFromBridge(address to, uint256 id)
public
onlyOwner {
_mint(to, id);
}
@inheritdoc IERC721Bridgeable
@dev In this implementation, the owner is the bridge by default. So `onlyOwner`
is enough.
*/
function burnFromBridge(uint256 id)
public
onlyOwner {
_burn(id);
}
@notice base URI for the tokens.
@dev By default empty, or perhaps we can target a default URI?
*/
function _baseURI()
internal
view
override
returns (string memory) {
return __baseURI;
}
function _setTokenURI(uint256 tokenId, string calldata _tokenURI) internal {
_requireMinted(tokenId);
_tokenURIs[tokenId] = _tokenURI;
}
function _setBaseURI(string memory baseURI_) internal {
__baseURI = baseURI_;
}
function baseURI() public view returns (string memory) {
return __baseURI;
}
function setBaseURI(string memory baseURI_) public onlyOwner {
_setBaseURI(baseURI_);
}
function setTokenURI(uint256 tokenId, string calldata _tokenURI) public onlyOwner {
_setTokenURI(tokenId, _tokenURI);
}
function mintWithTokenURI(address to, uint256 tokenId, string calldata _tokenURI) public onlyOwner returns (bool) {
mintFromBridge(to, tokenId);
_setTokenURI(tokenId, _tokenURI);
return true;
}
@notice A descriptive name for the token collection.
@dev `name()` must be overriden as the underlying value
is private, then not accessible for this contract (which extends
the base ERC721 contract).
*/
function name()
public
view
override
returns (string memory)
{
return _name;
}
@notice An abbreviated name for the token collection.
@dev `symbol()` must be overriden as the underlying value
is private, then not accessible for this contract (which extends
the base ERC721 contract).
*/
function symbol()
public
view
override
returns (string memory)
{
return _symbol;
}
}
Then comment out the setUp() function in Bridge.t.sol and add the following imports:
import "../src/token/ERC721BridgeableMetadata.sol";
import "../src/token/IERC721BridgeableMetadata.sol";
In order for us to see the generated request, please add the following lines at the end of Bridge::depositTokens (before emitting the DepositRequestInitiated event):
console.log(req.uri);
console.log(req.tokenURIs.length);
Also add the following import to Bridge.sol:
import { console } from "forge-std/Test.sol";
Now we can finally add the test function test_not_using_baseURI to Bridge.t.sol and execute it with forge test --match-test test_not_using_baseURI -vv.
function test_not_using_baseURI() public {
Users genusers = new Users();
address payable[] memory users = genusers.create(5);
alice = users[0];
bob = users[1];
erc721C1 = address(new ERC721BridgeableMetadata());
bytes memory initData = abi.encode("name", "symbol");
IERC721BridgeableMetadata(erc721C1).initialize(initData);
IERC721BridgeableMetadata(erc721C1).setBaseURI("http://example.com/");
IERC721Bridgeable(erc721C1).mintFromBridge(address(this), 1);
IERC721Bridgeable(erc721C1).mintFromBridge(address(this), 2);
snCore = address(new StarknetMessagingLocal());
address impl = address(new Starklane());
bytes memory dataInit = abi.encodeWithSelector(
Starklane.initialize.selector,
abi.encode(
address(this),
snCore,
0x1,
0x2
)
);
bridge = address(new ERC1967Proxy(impl, dataInit));
IStarklane(bridge).enableBridge(true);
uint256[] memory ids = new uint256[](2);
ids[0] = 1;
ids[1] = 2;
IERC721(erc721C1).approve(address(bridge), 1);
IERC721(erc721C1).approve(address(bridge), 2);
IStarklane(bridge).depositTokens{value : 1}(0x10, address(erc721C1), Cairo.snaddressWrap(100), ids, false);
}
This will give us an output looking similar to this:
[PASS] test_not_using_baseURI() (gas: 7052017)
Logs:
2
The empty line before the 2 shows that our uri (baseURI) is an empty string even though we set it in our testcase.
This shows that instead of packing our metadata into one baseURI we add two tokenURIs to the request.
Tools Used
Manual review
Recommended Mitigation
In order for the baseURI feature to work as expected, update the function signatures to include baseURI() or call it directly if the collection supports ERC721Metadata.
