[WP-M1] `TurboClerk.sol#getFeePercentageForSafe()` Wrong implementation makes the fees being mischarged when `customFeePercentageForSafe` or `customFeePercentageForCollateral` is set to `0`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/modules/TurboClerk.sol#L106-L121

Vulnerability details

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/modules/TurboClerk.sol#L106-L121

function getFeePercentageForSafe(TurboSafe safe, ERC20 collateral) external view returns (uint256) {
    // Get the custom fee percentage for the Safe.
    uint256 customFeePercentageForSafe = getCustomFeePercentageForSafe[safe];

    // If a custom fee percentage is set for the Safe, return it.
    if (customFeePercentageForSafe != 0) return customFeePercentageForSafe;

    // Get the custom fee percentage for the collateral type.
    uint256 customFeePercentageForCollateral = getCustomFeePercentageForCollateral[collateral];

    // If a custom fee percentage is set for the collateral, return it.
    if (customFeePercentageForCollateral != 0) return customFeePercentageForCollateral;

    // Otherwise, return the default fee percentage.
    return defaultFeePercentage;
}

Per the comment:

A fixed point number where 1e18 represents 100% and 0 represents 0%.

However, in getFeePercentageForSafe() when customFeePercentageForSafe or customFeePercentageForCollateral is 0, it's considered as not set yet. Which will then fallback to the default.

This can result in the fees being mischarged.

PoC

1. Clerk set defaultFeePercentage = 1E17 (10%)
2. Clerk set TurboSafe safe1's customFeePercentageForSafe = 0 (0%)
3. When safe1 earned 1,000 USDC and slurp():

Expected Results: Zero fee charged;

Actual Results: Charged for a 100 USDC fee.

Recommendation

Consider using type(uint256).max instead of 0 to represent 0%:

function getFeePercentageForSafe(TurboSafe safe, ERC20 collateral) external view returns (uint256) {
    // Get the custom fee percentage for the Safe.
    uint256 customFeePercentageForSafe = getCustomFeePercentageForSafe[safe];

    // If a custom fee percentage is set for the Safe, return it.
    if (customFeePercentageForSafe != 0) {
        if (customFeePercentageForSafe == type(uint256).max) return 0;

        return customFeePercentageForSafe;
    }

    // Get the custom fee percentage for the collateral type.
    uint256 customFeePercentageForCollateral = getCustomFeePercentageForCollateral[collateral];

    // If a custom fee percentage is set for the collateral, return it.
    if (customFeePercentageForCollateral != 0) {
        if (customFeePercentageForCollateral == type(uint256).max) return 0;

        return customFeePercentageForCollateral;
    }

    // Otherwise, return the default fee percentage.
    return defaultFeePercentage;
}

[transmissions11]

i see this more as a documentation issue, my preference would be to avoid using magic numbers and just have ppl set fees to 1 wei (will get rounded to 0 in practice)

[Joeysantoro]

Disputing severity to level 0 non-critical

[GalloDaSballo]

First of all, I want to commend the warden for the thorough work.

That said, ultimately the impact for this finding is minimal as the sponsor will have to use 1 instead of 0 to represent no fees.

Perhaps using a struct (bool, uint16) to mark percentages in BPS may be a solution that uses the same amount of storage slots.

That said, because the impact is minimal but the warden was able to find a valid exception to the ordinary flow of the logic, I believe Low Severity to be appropriate

[GalloDaSballo]

Because this is the only Low Severity finding from the Warden, I'm going to be judging it as a separate QA Report from Them