safeERC20Symbol() function will always revert when interating with tokens that returns bytes32 as Symbol

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/main/contracts/libraries/PanopticMath.sol#L85-L93

Vulnerability details

Impact

Tokens that return bytes32 as a Symbol are incompatible with the FactoryNFT.

Proof of Concept

Some tokens (e.g. MKR) have metadata fields (name / symbol) encoded as bytes32 instead of a string. Such tokens will cause that all calls to PanopticMath.safeERC20Symbol() ends up reverting the whole tx.

function safeERC20Symbol(address token) external view returns (string memory) {

    //@audit-issue => Any token that returns bytes32 will cause the tx to revert!

    // not guaranteed that token supports metadata extension
    // so we need to let call fail and return placeholder if not
    try IERC20Metadata(token).symbol() returns (string memory symbol) {
        return symbol;
    } catch {
        return "???";
    }
}

Coded PoC

Expand to see PoC

Add the below file under the folder test/foundry.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

// Foundry
import "forge-std/Test.sol";

import {PanopticMath} from "@libraries/PanopticMath.sol";
import {FactoryNFT} from "../../contracts/base/FactoryNFT.sol";

import {Pointer} from "@types/Pointer.sol";

import {IUniswapV3Pool} from "v3-core/interfaces/IUniswapV3Pool.sol";


//@audit => Short implementation of the Maker Token
//@audit => https://etherscan.io/address/0x9f8f72aa9304c8b593d555f12ef6589cc3a579a2#readContract#F7
contract DSToken {
    bytes32 public symbol;

    constructor(bytes32 symbol_) public {
        symbol = symbol_;
    }
}

contract TetherToken {
    string public symbol;
    
    constructor(string memory symbol_) public {
        symbol = symbol_;
    }

}

contract UniPool {
  address public token0;
  address public token1;

  constructor(address _token0, address _token1) {
    token0 = _token0;
    token1 = _token1;
  }
}

contract PanopticPool {
  IUniswapV3Pool pool;

  constructor(address _uniPool) {
    pool = IUniswapV3Pool(_uniPool);
  }
}

contract FactoryNFTMock is FactoryNFT {
  constructor(
      bytes32[] memory properties,
      uint256[][] memory indices,
      Pointer[][] memory pointers
  )
      FactoryNFT(properties, indices, pointers){}
    
    function callsafeERC20Symbol(address token) external view returns (string memory) {
        PanopticMath.safeERC20Symbol(token);
    }
}


contract SafeERC20SymbolTest is Test {
    bytes32[] properties;
    uint256[][] indices;
    Pointer[][] pointers;

    function test_safeERC20Symbol() external {
      DSToken makerToken = new DSToken(bytes32(0x4d4b520000000000000000000000000000000000000000000000000000000000));
      TetherToken usdtToken = new TetherToken(string("USDT"));
      
      UniPool uniPool = new UniPool(address(makerToken), address(usdtToken));

      PanopticPool panopticPool = new PanopticPool(address(uniPool));

      FactoryNFTMock factoryNFT = new FactoryNFTMock(properties, indices, pointers);

      //@audit-info => All fine when symbol returns a string!
      string memory usdtSymbol = factoryNFT.callsafeERC20Symbol(address(usdtToken));

      //@audit => Execution reverts when symbol returns bytes32
      vm.expectRevert();
      string memory makerSymbol = factoryNFT.callsafeERC20Symbol(address(makerToken));

      
      address panopticPoolAddress = address(panopticPool);
      uint256 tokenId = uint256(uint160(panopticPoolAddress));

      //@audit => Execution reverts when symbol returns bytes32
      vm.expectRevert();
      string memory tokenURI = factoryNFT.tokenURI(tokenId);

    }
}

Run the PoC with the command forge test --match-test test_safeERC20Symbol -vvvv and analize the output, as expected, the two calls to the MKR mock token, (returns bytes32) causes the tx to revert

    │   ├─ [2702] PanopticMath::safeERC20Symbol(TetherToken: [0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63]) [delegatecall]
    │   │   ├─ [1147] TetherToken::symbol() [staticcall]
    │   │   │   └─ ← [Return] "USDT"
    │   │   └─ ← [Return] "USDT"
    │   └─ ← [Return] ""
    
    //@audit-issue => First call to the MKR mock token
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← [Return]
    ├─ [1660] FactoryNFTMock::callsafeERC20Symbol(DSToken: [0xFEfC6BAF87cF3684058D62Da40Ff3A795946Ab06]) [staticcall]
    │   ├─ [1014] PanopticMath::safeERC20Symbol(DSToken: [0xFEfC6BAF87cF3684058D62Da40Ff3A795946Ab06]) [delegatecall]
    │   │   ├─ [261] DSToken::symbol() [staticcall]
    │   │   │   └─ ← [Return] ""
    │   │   └─ ← [Revert] EvmError: Revert
    │   └─ ← [Revert] EvmError: Revert

    //@audit-issue => Second call to the MKR mock token
    ├─ [0] VM::expectRevert(custom error f4844814:)
    │   └─ ← [Return]
    ├─ [753] FactoryNFTMock::tokenURI(47508985303868808400900336068535627141182218264 [4.75e46]) [staticcall]
    │   ├─ [24] PanopticPool::univ3pool() [staticcall]
    │   │   └─ ← [Revert] EvmError: Revert
    │   └─ ← [Revert] EvmError: Revert
    └─ ← [Stop]

Tools Used

Manual Audit

Recommended Mitigation Steps

Make a low-level call to retrieve the symbol and then convert it to string.

Assessed type

ERC20

[Picodes]

MKR isn't following the ERC20Metadata interface so it's more an issue with MKR than with Panoptic

[c4-judge]

Picodes marked the issue as unsatisfactory:
Invalid

[stalinMacias]

Hello Judge @Picodes
I'd like to ask if this report could be considered as part of my QA report since the safeERC20Symbol() function is meant to pull the symbol of the token, regardless of whether the queried token supports metadata extension or not.
When interacting with tokens that return bytes32, the try-catch logic is broken, instead of being able to return ??? as expected.

[Picodes]

@stalinMacias This could have been a NC comment but here there is no QA pool, so I'll wait for clarification from C4's team before handling this

[stalinMacias]

@Picodes Thanks for the response. I'll be looking forward to hearing what C4's team says about it.

[stalinMacias]

@liveactionllama @Picodes I've just seen that labels for final results have been assigned on reports.
I just wanted to ping on this report to know if it will be assessed as QA and included in the final report?

[Picodes]

@stalinMacias following the team request I was asked to select the 3 best QA reports, and I didn't pick this one.

Thanks for your understanding!

[stalinMacias]

@Picodes Thanks for the answer. I do understand. Just wondering if, since team asked to pick up 3 reports, and #38 and #30 are a dupe of each other, if this could be the 3rd one :) ? For example:

1. s_poolAssets underflow in CollateralTracker.sol will lead to protocol failure #38 & Lack of overflow validation allows manipulation of s_poolAssets leading to incorrect totalAssets calculation #30
2. Users solvency validation are being erroneously executed since they are done on the basis of wrong tick data #26
3. This one. safeERC20Symbol() function will always revert when interating with tokens that returns bytes32 as Symbol #17

[Picodes]

Unfortunately they count as distinct reports

[stalinMacias]

@Picodes okaay, all good, thanks for answering my questions 🫡