Missing return values in `assertMinCollateralValues` function causes difficulty in slippage checks

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/PanopticPool.sol#L329-L336

Vulnerability details

Impact

The function assertMinCollateralValues is designed to check if the user's collateral balances for two different tokens meet certain minimum requirements specified by minValue0 and minValue1. However, if minValue0 and minValue1 are both set to 0, the function will always assert true, even if the user has zero collateral balances or non-zero collateral balances. This function will not return any return values on asserting true, Causing a difficulty in slippage check.Mainly when using multiCall to check the slippage as this fuction will not return any values, causing the return values always be 0 bytes on success and we cannot decode any values from it.

Proof of Concept

Impact scenario:

1. The assertMinCollateralValues function is called with minValue0 and minValue1 set to 0.
2. The function will always assert true, even if the user has zero collateral balances or non-zero collateral balances.
3. When a multicall is used to check the slippage, the function will not return any values, causing the return values always be 0 bytes on success and we cannot decode any values from it.
4. This will cause a difficulty in checking the slippage.

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.18;

import {Test, console} from "forge-std/Test.sol";
import {FactoryNFT} from "@base/FactoryNFT.sol";
import {CollateralTrackerTest} from "./core/CollateralTracker.t.sol";
import {CollateralTracker} from "@contracts/CollateralTracker.sol";
import {IERC20Partial} from "@tokens/interfaces/IERC20Partial.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/ERC20Mock.sol";
import {PanopticFactoryTest} from "./core/PanopticFactory.t.sol";
import {PanopticPool} from "@contracts/PanopticPool.sol";
import {PanopticPoolTest} from "./core/PanopticPool.t.sol";
import {SemiFungiblePositionManager} from "@contracts/SemiFungiblePositionManager.sol";

contract BugTest_CollateralTracker is CollateralTrackerTest {
    FactoryNFT public factory;
    ERC20Mock public mock1;
    ERC20Mock public mock2;
    address public mockAddr1;
    address public mockAddr2;
    address public ctAddr;
    address public ppAddr;
    CollateralTracker public ct;
    PanopticPool public pp;
    SemiFungiblePositionManager public sf;

    function setUp() public override(CollateralTrackerTest) {
        sf = new SemiFungiblePositionManager(V3FACTORY);
        mock1 = new ERC20Mock("Mock1", "MCK1", address(this), 1e18);
        mock2 = new ERC20Mock("Mock2", "MCK2", address(this), 1e18);
        ct = new CollateralTracker(10, 2_000, 1_000, -1_024, 5_000, 9_000, 20_000);
        pp = new PanopticPool(sf);
        mock1.approveInternal(address(this), address(ct), type(uint256).max);
        mock2.approveInternal(address(this), address(ct), type(uint256).max);
        mockAddr1 = address(mock1);
        mockAddr2 = address(mock2);
        ctAddr = address(ct);
        ppAddr = address(pp);
    }
    function test_assertMinCollateral_Asserts0() public {
        CollateralTracker ct0 = new CollateralTracker(10, 2_000, 1_000, -1_024, 5_000, 9_000, 20_000);
        address ct0Addr = address(ct0);
        ct0.startToken(true, mockAddr1, mockAddr2, fee, pp);
        CollateralTracker ct1 = new CollateralTracker(10, 2_000, 1_000, -1_024, 5_000, 9_000, 20_000);
        address ct1Addr = address(ct1);
        ct1.startToken(false, mockAddr1, mockAddr2, fee, pp);
        pp.startPool(USDC_WETH_5, mockAddr1, mockAddr2, ct0, ct1);

        mock1.mint(Bob, 10e18);
        mock2.mint(Bob, 10e18);

        vm.startPrank(Bob);
        mock1.approveInternal(Bob, ct0Addr, type(uint256).max);
        mock2.approveInternal(Bob, ct1Addr, type(uint256).max);
        ct0.deposit(10e18, Bob);
        ct1.deposit(10e18, Bob);
        vm.stopPrank();

        // asserts for 0 collateral balances
        vm.startPrank(Alice);
        pp.assertMinCollateralValues(0, 0);
        vm.stopPrank();

        // asserts for non 0 collateral balances
        vm.startPrank(Bob);
        pp.assertMinCollateralValues(0, 0);
        vm.stopPrank();

        // Doesnt return any values during the multicall
        vm.startPrank(Bob);
        bytes[] memory data = new bytes[](1);
        data[0] = abi.encodeWithSignature("assertMinCollateralValues(uint256,uint256)", 0, 0);
        pp.multicall(data);
        // will always returns 0X0 if the function succeeds
        // thus we cannot decode the return values
        vm.stopPrank();
    }
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

It is recommended to return a value on asserting true in the assertMinCollateralValues function. This will help in decoding the return values during the multicall and to check the slippage.

Here is the recommended mitigation:

- function assertMinCollateralValues(uint256 minValue0, uint256 minValue1) external view {
+ function assertMinCollateralValues(uint256 minValue0, uint256 minValue1) external view returns (bool) {
        CollateralTracker ct0 = s_collateralToken0;
        CollateralTracker ct1 = s_collateralToken1;
        if (
            ct0.convertToAssets(ct0.balanceOf(msg.sender)) < minValue0
                || ct1.convertToAssets(ct1.balanceOf(msg.sender)) < minValue1
        ) revert Errors.NotEnoughCollateral();
+       return true;
    }

Assessed type

Other

[Picodes]

It reverts on failure so you need to use this

[c4-judge]

Picodes marked the issue as unsatisfactory: Insufficient proof