Vault721.tokenURI does not comply with ERC721 - Metadata specification

mediumCode4rena

Lines of code

https://github.com/open-dollar/od-contracts/blob/f4f0246bb26277249c1d5afe6201d4d9096e52e6/src/contracts/proxies/Vault721.sol#L140

Vulnerability details

Impact

According to the standard, the tokenURI method must be reverted if a non-existent tokenId is passed. In the Vault721 contract, this point was ignored. What leads to a violation of the EIP721 spec

Proof of Concept

https://github.com/open-dollar/od-contracts/blob/v1.5.5-audit/src/contracts/proxies/Vault721.sol#L140-L142

js📋
File: contracts/proxies/Vault721.sol

  /**
   * @dev generate URI with updated vault information
   */
  function tokenURI(uint256 _safeId) public view override returns (string memory uri) {
    // @audit should throw if safeId does not exist according to the ERC721-Metadata specification
    uri = nftRenderer.render(_safeId);
  }

The responsibility for checking whether a token exists may be put on the nftRenderer depending on the implementation, and may also be missing, changed or added in the future. But the underlying Vault721 contract that is supposed to comply with the standard does not follow the specification

Similar problem with violation of ERC721 specification: https://github.com/code-423n4/2023-04-caviar-findings/issues/44

Tools Used

• Manual review
• https://eips.ethereum.org/EIPS/eip-721#specification

Recommended Mitigation Steps

Add a token existence check at the Vault721 level, example:

js📋
  /**
   * @dev generate URI with updated vault information
   */
  function tokenURI(uint256 _safeId) public view override returns (string memory uri) {
+++ _requireMinted(_safeId);
    uri = nftRenderer.render(_safeId);
  }

Assessed type

ERC721