Cyfrin / foundry-defi-stablecoin-cu

250 stars 117 forks source link

Handling Arithmetic overflow/underflow at invariant testing #68

Closed 0fprod closed 7 months ago

0fprod commented 7 months ago

Hi! I've already seen the closed issue https://github.com/Cyfrin/foundry-defi-stablecoin-f23/issues/30 but is not my case. Im running into this error when calling depositCollateral in the handler contract with a very high _amount.

This is the handler contract

contract DSCProtocolHandler is StdInvariant, Test {
    DSCoin dsc;
    DSCEngine engine;
    ConfigHelper.Configuration config;
    address[] allowedCollaterals = new address[](2);
    address alice;

    constructor(address _dsc, address _engine, ConfigHelper.Configuration memory _config, address _alice) {
        dsc = DSCoin(_dsc);
        engine = DSCEngine(_engine);
        config = _config;
        allowedCollaterals[0] = _config.wETHaddress;
        allowedCollaterals[1] = _config.wBTCaddress;
        alice = _alice;
    }

    function depositCollateral(uint _collateralIndex, uint _amount) public {
        uint index = bound(_collateralIndex,0, 1);
        address collateral = allowedCollaterals[index]; 
        _amount = bound(_amount, 1, type(uint).max);

        vm.startPrank(alice);
        ERC20Mock(collateral).mint(alice, _amount);
        ERC20Mock(collateral).approve(address(engine), _amount);
        engine.depositCollateral(collateral, _amount);
        vm.stopPrank();
    }
}

This is the invariant contract

contract DSCEngineInvariants is StdInvariant, Test {
    DSCoin dsc;
    DSCEngine engine;
    DSCProtocolHandler handler;
    ConfigHelper.Configuration config;
    address wETH;
    address wBTC;
    address alice = makeAddr("alice");

    function setUp() external {
      address owner = makeAddr("owner");
      ConfigHelper helperConfig = new ConfigHelper(); 
      config = helperConfig.getTokensAndPriceFeeds();
      address[] memory tokens = new address[](2);
      tokens[0] = config.wETHaddress;
      tokens[1] = config.wBTCaddress;
      address[] memory priceFeeds = new address[](2);
      priceFeeds[0] = config.wETHPriceFeedAddress;
      priceFeeds[1] = config.wBTCPriceFeedAddress;
      wETH = config.wETHaddress;
      wBTC = config.wBTCaddress;

      dsc = new DSCoin(owner);
      engine = new DSCEngine(tokens, priceFeeds, address(dsc));
      handler = new DSCProtocolHandler(address(dsc), address(engine), config, alice);
      targetContract(address(handler));
    }

    function invariant_protocolMustHaveMoreValueThanTotalSupply() public view {
      uint totalSupply = dsc.totalSupply();
      uint wETHDeposited = IERC20(wETH).balanceOf(address(engine));
      uint wBTCDeposited = IERC20(wBTC).balanceOf(address(engine));

      uint wETHValue = engine.getUSDValue(config.wETHPriceFeedAddress, wETHDeposited);
      uint wBTCValue = engine.getUSDValue(config.wBTCPriceFeedAddress, wBTCDeposited);

      assert(wETHValue + wBTCValue >= totalSupply);
    }
}

It fails when engine.getUSDValue(collateralAddres, amountDeposited); is called. After debugging a bit with console logs I've seen that with a very high number it throws this exception. This is the function implementation


**
     * @dev Returns the USD value of a given token amount.
     * @param _priceFeedAddress The address of the Chainlink price feed.
     * @param _amount The amount of tokens.
     * @return The USD value of the token amount.
     */
    function getUSDValue(address _priceFeedAddress, uint256 _amount) public view returns (uint256) {
        AggregatorV3Interface priceFeed = AggregatorV3Interface(_priceFeedAddress);
        (, int256 price,,,) = priceFeed.latestRoundData();

        if (_amount == 0 || price == 0) {
            return 0;
        }
        console2.log("We're about to revert with Arithmetic over/underflow");
        uint256 priceWithPrecision = uint256(price) * ADDITIONAL_FEED_PRECISION;
        console.log("### ~ priceWithPrecision:", priceWithPrecision);
        console.log("### ~ _amount:", _amount);
        uint256 usdValue = (priceWithPrecision * _amount) / PRECISION;
        console.log("### ~ usdValue:", usdValue);       //  --> This is never printed when priceWithPrecision * _amount with high values happens
        return usdValue;
    }

Debugging

This is what the console prints: (the pricefeeds are mocked to return 2k weth abd 40k wbtc)

image

So I took those numbers and tested it using chisel

image

More traces

image

How should I handle this scenario? In any case if the price in USD is too high or the amount deposited is too high... this is going to break right? 🤔

Thanks

PD: This is my repository if you want to see more context https://github.com/0fprod/defi-protocol

EDIT:

I've tried lowering the _amount = bound(_amount, 1, type(uint).max); to _amount = bound(_amount, 1,type(uint96).max); And it does not revert [PASS] invariant_protocolMustHaveMoreValueThanTotalSupply() (runs: 64, calls: 4096, reverts: 0) but the question still the same, in a "real" scenario this overflow error still could happen right?

0fprod commented 7 months ago

I guess that its impossible to happen because nobody has that amount of tokens. 😄 ?