Tendency - Flawed Initialization in `BigBang` Contract: `minMintFeeStart` Exceeds `maxMintFeeStart`

[sherlock-admin3]

Tendency

medium

Flawed Initialization in BigBang Contract: minMintFeeStart Exceeds maxMintFeeStart

Summary

minMintFeeStart is wrongly hardcoded to a value greater than the maxMintFeeStart in BigBang

Vulnerability Detail

The maxMintFeeStart and minMintFeeStart are hardcoded during initialization to these values:

        maxMintFeeStart = 975000000000000000; // 0.975 *1e18
        minMintFeeStart = 1000000000000000000; // 1*1e18

Notice the min here is set to a value greater than the max, I believe this mistake could have been due to the error in BBLendingCommon::_computeVariableOpeningFee function computation:

        uint256 fee = maxMintFee
            - (((_exchangeRate - maxMintFeeStart) * (maxMintFee - minMintFee)) / (minMintFeeStart - maxMintFeeStart));

Here, the maxMintFeeStart is subtracted from minMintFeeStart, when the expected behaviour is that maxMintFeeStart always be greater than the minMintFeeStart, as seen in BigBang::setMinAndMaxMintRange function:

    function setMinAndMaxMintRange(uint256 _min, uint256 _max) external onlyOwner {
        emit UpdateMinMaxMintRange(minMintFeeStart, _min, maxMintFeeStart, _max);

        if (_min >= _max) revert NotValid();

        minMintFeeStart = _min;
        maxMintFeeStart = _max;
    }

Impact

When the contract owner updates the max and min MintFeeStart with BigBang::setMinAndMaxMintRange function, the max will be set to a value greater than the min, this will completely brick all Borrow operations, since _computeVariableOpeningFee will always revert due to an underflow at:

        uint256 fee = maxMintFee
            - (((_exchangeRate - maxMintFeeStart) * (maxMintFee - minMintFee)) / (minMintFeeStart - maxMintFeeStart));

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/Tapioca-bar/contracts/markets/bigBang/BigBang.sol#L194-L195

Tool used

Manual Review

Recommendation

Update the maxMintFeeStart and minMintFeeStart:

        maxMintFeeStart = 1000000000000000000;
        minMintFeeStart = 975000000000000000; 

Then update _computeVariableOpeningFee function to subtract the min from the max during computation:

        uint256 fee = maxMintFee
            - (((_exchangeRate - maxMintFeeStart) * (maxMintFee - minMintFee)) / (maxMintFeeStart - minMintFeeStart));

Duplicate of #119

[cryptotechmaker]

Invalid; it refers to the delta of USDO from 1USD

[nevillehuang]

Invalid, misunderstanding of variables. I believe this variables refers to the delta/deviation allowed for USDO, and it is always enforced that minMintFeeStart >= maxMintFeeStart

[Haxatron]

Escalate

While the Watson have a misunderstanding that minMintFeeStart >= maxMintFeeStart, I believe the following part highlighted is valid:

Here, the maxMintFeeStart is subtracted from minMintFeeStart, when the expected behaviour is that maxMintFeeStart always be greater than the minMintFeeStart, as seen in BigBang::setMinAndMaxMintRange function:

function setMinAndMaxMintRange(uint256 _min, uint256 _max) external onlyOwner {
    emit UpdateMinMaxMintRange(minMintFeeStart, _min, maxMintFeeStart, _max);

    if (_min >= _max) revert NotValid();

    minMintFeeStart = _min;
    maxMintFeeStart = _max;
}

Whereby the admin cannot change the minMintFeeStart to be greater than maxMintFeeStart .

[sherlock-admin2]

Escalate

While the Watson have a misunderstanding that minMintFeeStart >= maxMintFeeStart, I believe the following part highlighted is valid:

Here, the maxMintFeeStart is subtracted from minMintFeeStart, when the expected behaviour is that maxMintFeeStart always be greater than the minMintFeeStart, as seen in BigBang::setMinAndMaxMintRange function:

function setMinAndMaxMintRange(uint256 _min, uint256 _max) external onlyOwner {
    emit UpdateMinMaxMintRange(minMintFeeStart, _min, maxMintFeeStart, _max);

    if (_min >= _max) revert NotValid();

    minMintFeeStart = _min;
    maxMintFeeStart = _max;
}

Whereby the admin cannot change the minMintFeeStart to be greater than maxMintFeeStart .

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

@cryptotechmaker Could you take a look, I think my initial comment is inaccurate. I believe there indeed are some inconsistencies here, unless maxMintFeeStart and minMintFeeStart is intended to be never changed

[maarcweiss]

I think it will not be changed, but the check if (_min >= _max) revert NotValid(); is wrong, should be otherwise.

[nevillehuang]

I think this issue could be valid, given it will permanently prevent a change initiated via setMinAndMaxMintRange. However, if the current hardcoded deviation of USDO (which is a stablecoin) is never intended to be changed, this could be low severity since no core contract functionality is blocked in that case.

[Haxatron]

Given that there is a function named setMinAndMaxMintRange(uint256) that cannot work properly, I believe this to be a core contract functionality that is impacted and therefore a valid medium bug.

[nevillehuang]

@Haxatron I can agree with that, I leave the decision up to @Czar102

[cvetanovv]

I tend to agree with escalation that this is a valid Medium because minMintFeeStart and maxMintFeeStart are hardcoded wrong and setMinAndMaxMintRange() cannot fix the problem.

[cvetanovv]

Planning to accept the escalation and make this issue a valid Medium.

[maarcweiss]

I tend to agree with escalation that this is a valid Medium because minMintFeeStart and maxMintFeeStart are hardcoded wrong and setMinAndMaxMintRange() cannot fix the problem.

The variables are not hardcoded wrong, the check on the setMinAndMaxMintRange function is, so they can't be updated. I think it is not planned to be changed though. @cryptotechmaker wdyt?

[Tendency001]

I believe there can't be a setter functionality for this values then have us assume the values wouldn't be changed. The fact that the functionality exists means there is a possibility, and we audited with that in mind.

[cvetanovv]

I tend to agree with escalation that this is a valid Medium because minMintFeeStart and maxMintFeeStart are hardcoded wrong and setMinAndMaxMintRange() cannot fix the problem.

The variables are not hardcoded wrong, the check on the setMinAndMaxMintRange function is, so they can't be updated. I think it is not planned to be changed though. @cryptotechmaker wdyt?

@maarcweiss I apologize for not expressing myself correctly. I mean they are hardcoded because setMinAndMaxMintRange() doesn't work properly.

They are supposed to be changed by setMinAndMaxMintRange() which doesn't work and this according to the documentation is Medium: "Breaks core contract functionality". But if you don't plan to change them at all now or in the future, then the report may remain invalid.

[maarcweiss]

I tend to agree with escalation that this is a valid Medium because minMintFeeStart and maxMintFeeStart are hardcoded wrong and setMinAndMaxMintRange() cannot fix the problem.

The variables are not hardcoded wrong, the check on the setMinAndMaxMintRange function is, so they can't be updated. I think it is not planned to be changed though. @cryptotechmaker wdyt?

@maarcweiss I apologize for not expressing myself correctly. I mean they are hardcoded because setMinAndMaxMintRange() doesn't work properly.

They are supposed to be changed by setMinAndMaxMintRange() which doesn't work and this according to the documentation is Medium: "Breaks core contract functionality". But if you don't plan to change them at all now or in the future, then the report may remain invalid.

Thanks, will circle back with the team on the willingness to change them.

[nevillehuang]

USDO is a stable coin that parameters may well be kept forever at that value. If not, something must have gone seriously wrong such that the whole protocol would have to be paused if the value of USDO would have dropped pass this thresholds. But there indeed is a inconsistency here so I leave it up to sponsor and @cvetanovv to decide, would have no qualms about this issue being valid.

[maarcweiss]

The submitter did not understand the context, also I think it should be duped with: https://github.com/sherlock-audit/2024-02-tapioca-judging/issues/119

[Tendency001]

While it's true that I did not understand the context, I still clearly showed an issue that should be fixed. This issue was accepted just because the submitted also made mention of "blacklist" by @Czar102 If this issue is judged based on context and deemed invalid then there will be an inconsistency in judging here

[cvetanovv]

Although this issue and #119 are not completely correct in their description, both issues manage to catch the main problem of the setMinAndMaxMintRange() function.

So I have decide to accept the escalation to make this issue Medium and duplicate #27 and #119 as #119 will be the main report because of the better recommendation.

[nevillehuang]

@cvetanovv good catch #119 indeed should be a duplicate based on the following and it also explores the potential full impact.

This will make it impossible to properly update the maxMintFeeStart and minMintFeeStart to have proper values because if it is enforced that maxMintFeeStart > than minMintFeeStart, then _computeVariableOpeningFee() will always enter the first if (_exchangeRate >= minMintFeeStart) and wrongly return the minimum fee.

[Evert0x]

Result: Medium Duplicate of #119

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• haxatron: accepted