howlbot-integration[bot] commented 1 month ago

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](https://github.com/code-423n4/2024-06-panoptic/tree/main/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 commented 1 month ago

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