`calculateFee()` will always return zero due to incorrect `feeBPS`

Summary

calculateFee() will always return zero due to incorrect feeBPS

Vulnerability Detail

Impact

Issue 1: A basis point is a common unit of measure for interest rates and other percentages in finance. Basis points are typically expressed with the abbreviations bp, bps, or bips.

In LiquidationRow.sol, feeBps is set to 0 which is given below,

45    /// @notice Fee in basis points (bps). 1 bps is 0.01%
46    uint256 public feeBps = 0;

Now, check this Natspec at L-45, Per Natspec the feeBps must be 1 bps or 0.01%. With current implementation, the calculateFee() will always return 0


    function calculateFee(uint256 amount) public view returns (uint256) {
>>        return (amount * feeBps) / MAX_PCT;
    }

Since the feeBPS is hardcoded to 0 therefore the calculateFee() will always return 0.

Issue 2: In _performLiquidation(),


    function _performLiquidation(
        uint256 gasBefore,
        address fromToken,
        address asyncSwapper,
        IDestinationVault[] memory vaultsToLiquidate,
        SwapParams memory params,
        uint256 totalBalanceToLiquidate,
        uint256[] memory vaultsBalances
    ) private {

      // some code

        // if the fee feature is turned on, send the fee to the fee receiver
>>      if (feeReceiver != address(0) && feeBps > 0) {

      // some code

    }

To perform the if condition it needs feeBps > 0 which is not possible with current implementation as feeBPS = 0. This function wont work as expected which is an undesired behaviour.

Issue 3: feeBPS can be set to 0 Therefore the discussed condition in Issue 2 can not be prevented.


    function setFeeAndReceiver(address _feeReceiver, uint256 _feeBps) external hasRole(Roles.LIQUIDATOR_ROLE) {
        // feeBps should be less than or equal to MAX_PCT (100%) to prevent overflows
        if (_feeBps > MAX_PCT) revert FeeTooHigh();

        feeBps = _feeBps;
        // slither-disable-next-line missing-zero-check
        feeReceiver = _feeReceiver;
    }

A input validation is required to prevent this issue.

To learn further on BPS, checkout this link. Below is the table showing BPS in percentage. MAX_PCT = 10_000 is correctly set to 100%, however feeBps must be set to 1 BPS per Natspec, see recommendations too.

bps

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/liquidation/LiquidationRow.sol#L46

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/liquidation/LiquidationRow.sol#L100

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/liquidation/LiquidationRow.sol#L254

Tool used

Manual Review

Recommendation

The issues can be fixed with below recommendations,


    /// @notice Fee in basis points (bps). 1 bps is 0.01%
-    uint256 public feeBps = 0;
+    uint256 public feeBps = 1;

     // some code

    function setFeeAndReceiver(address _feeReceiver, uint256 _feeBps) external hasRole(Roles.LIQUIDATOR_ROLE) {
+      require(_feeBps != 0, "feeBPS can not be zero");
+      require(_feeReceiver != address(0), "feeReceiver can not be zero address");
        // feeBps should be less than or equal to MAX_PCT (100%) to prevent overflows
        if (_feeBps > MAX_PCT) revert FeeTooHigh();

        feeBps = _feeBps;
        // slither-disable-next-line missing-zero-check
        feeReceiver = _feeReceiver;
    }

    function calculateFee(uint256 amount) public view returns (uint256) {
+     require(amount != 0, "amount should be greater than zero");
        return (amount * feeBps) / MAX_PCT;
    }

sherlock-audit / 2023-06-tokemak-judging

MohammedRizwan - `calculateFee()` will always return zero due to incorrect `feeBPS` #195