Wrong input validation in `PoolManager::setParamsLASA` function

[hats-bug-reporter[bot]]

Github username: @JacoboLansac Twitter username: jacolansac Submission hash (on-chain): 0xe1a5ff1289ee81f3df75759110b2c2817c4cf3def2979d069c0536cc1c5c566c Severity: medium

Description: Description\

The following checks are performed inside setParamsLASA:

    function setParamsLASA( 

        // ...

        _validateParameter(_upperBoundMaxRate, UPPER_BOUND_MAX_RATE);

        _validateParameter(_lowerBoundMaxRate, LOWER_BOUND_MAX_RATE);

        _validateParameter(_lowerBoundMaxRate, _upperBoundMaxRate);

The function _validateParameter() checks if the first argument is higher than the second argument. This check makes sense for two of the above checks, but it does not make sense for _upperBoundMaxRate, as it should be lower than the UPPER_BOUND_MAX_RATE

    function _validateParameter(uint256 _parameterValue, uint256 _parameterLimit) internal pure {
        if (_parameterValue > _parameterLimit) {
            revert InvalidAction();
        }
    }

Recommedation

    function setParamsLASA( 

        // ...

-       _validateParameter(_upperBoundMaxRate, UPPER_BOUND_MAX_RATE);
+         if (_upperBoundMaxRate < UPPER_BOUND_MAX_RATE) {
            revert InvalidAction();

Severity Low

[JacoboLansac]

Sorry I submitted the fix too quickly because the competition was closing. The correct recomended fix should be:

    function setParamsLASA(
        // ...

-       _validateParameter(_upperBoundMaxRate, UPPER_BOUND_MAX_RATE);
+       require(_upperBoundMaxRate < UPPER_BOUND_MAX_RATE, "_upperBoundMaxRate too high");

        _validateParameter(_lowerBoundMaxRate, LOWER_BOUND_MAX_RATE);

        _validateParameter(_lowerBoundMaxRate, _upperBoundMaxRate);

        // ...

[vm06007]

orignal code:

if (_parameterValue > _parameterLimit) {
    revert InvalidAction();
}

your suggestion:

require(_upperBoundMaxRate < UPPER_BOUND_MAX_RATE, "_upperBoundMaxRate too high");

there is no difference between these: in our case we are validating that first parameter should be below the second, if first parameter is higher than the second parameter we revert the transaction.

as you've said: _upperBoundMaxRate, as it should be lower than the UPPER_BOUND_MAX_RATE

our condition _validateParameter(_upperBoundMaxRate, UPPER_BOUND_MAX_RATE); - ensures that _upperBoundMaxRate is lower than UPPER_BOUND_MAX_RATE

if _upperBoundMaxRate is higher than UPPER_BOUND_MAX_RATE then function reverts

[vm06007]

@JacoboLansac please see my response above and let us know if you agree that no code change is needed 👓 👓.👓 maybe you've misunderstood the verification function

require and if are replaceable and just reverse the check, you have require and want to use if -> then you just need to reverse condition, and other way around, also using if is slightly cheaper than using require.

[JacoboLansac]

I apologise. I saw this piece of code with only 4 minutes left until contest closing time, and I submitted a shitty report.

Your message does make sense regarding the upper bound check. The upper bound check is correct. However, the lower boundary check is the one that is incorrect.

Simply put, _validateParameter() cannot be used to validate both lower and upper boundaries, as the validation goes in opposite directions. (I reported the wrong check because of going too fast, my bad). The one that is incorrect is the lower boundary check, because it reverts if the _lowerBoundMaxRate > LOWER_BOUND_MAX_RATE, which should always be the case. Does it make sense?

If you agree that this issue is valid, I can open another issue to have a cleaner discussion thread. Lemme know what you think.

[JacoboLansac]

I checked your tests, and the reason why the issue was not caught is because the only tested values are exactly the boundaries. Example from scalingAlgorithm.test.js:

        const newMulfactor = toWei("0.5");
        const newUpperBound = toWei("300");  // UPPER_BOUND_MAX_RATE == 300
        const newLowerBound = toWei("100");  // LOWER_BOUND_MAX_RATE == 100

        const NORMALISATION_FACTOR = 4838400;

        await contracts.lending.setParamsLASA(
            WETH.address,
            newMulfactor,
            newUpperBound,
            newLowerBound,
            true,
            false
        );

And since _validateParameter() does not revert if the value is exactly the same as the boundary, the check passes. However, if you try to run the same tests, but different values presumably inside the acceptable range, it will revert:

        const newMulfactor = toWei("0.5");
        const newUpperBound = toWei("250");  // UPPER_BOUND_MAX_RATE == 300
        const newLowerBound = toWei("150");  // LOWER_BOUND_MAX_RATE == 100

        const NORMALISATION_FACTOR = 4838400;

        // this will revert due to a wrong check on the newLowerBound 
        await contracts.lending.setParamsLASA(
            WETH.address,
            newMulfactor,
            newUpperBound,
            newLowerBound,
            true,
            false
        );

Updated recommendation

With all that info in mind, the updated recommendation would be:

    function setParamsLASA(
        // ...

        _validateParameter(_upperBoundMaxRate, UPPER_BOUND_MAX_RATE);

-       _validateParameter(_lowerBoundMaxRate, LOWER_BOUND_MAX_RATE);
+       _validateParameter(LOWER_BOUND_MAX_RATE, _lowerBoundMaxRate);

        _validateParameter(_lowerBoundMaxRate, _upperBoundMaxRate);

        // ...

[JacoboLansac]

@vm06007 You can close this issue, I will open a new one in the private competition with the right description and right fix. Thanks for your patience

[vm06007]

Your message does make sense regarding the upper bound check. The upper bound check is correct. However, the lower boundary check is the one that is incorrect.

first please let me know if you really understand this function you are talking about, because even from your second reply you are still wrong @JacoboLansac

You are saying: Your message does make sense regarding the upper bound check. The upper bound check is correct. However, the lower boundary check is the one that is incorrect.

In fact it is still not correct and the code change you are proposing is wrong.

Original code: _validateParameter(_lowerBoundMaxRate, LOWER_BOUND_MAX_RATE); // here we are validating that IF _lowerBoundMaxRate is GREATER than LOWER_BOUND_MAX_RATE - we must REVERT

So in other words it is checking that _lowerBoundMaxRate value MUST be lower than LOWER_BOUND_MAX_RATE limit (maximum possible rate for lower bound)

So you are still wrong and I would like you to focus better on code before submitting anything else.

[vm06007]

Here is an example:

LowerBound rate must be between 0 and 10 and LOWER_BOUND_MAX_RATE is guarding that so it can only be set between 0 and 10.

Hence we set LOWER_BOUND_MAX_RATE to 10 and it will protect the parameter and revert if passed value for _lowerBoundMaxRate is higher than the one it is compared with.

[vm06007]

The one that is incorrect is the lower boundary check, because it reverts if the _lowerBoundMaxRate > LOWER_BOUND_MAX_RATE, which should always be the case. Does it make sense?

No it does not make sense. It should revert if parameter provided for _lowerBoundMaxRate is higher than LOWER_BOUND_MAX_RATE

Here are the boundaries: https://wisesoft.gitbook.io/wise/wiselending.com/lasa-ai

There is upper boundary with its own maximum value There is lower boundary with its own maximum value

Then we also check that lower boundary does not cross upper boundary by making final check _validateParameter(_lowerBoundMaxRate, _upperBoundMaxRate);

[JacoboLansac]

Got it. I clearly misunderstood it. Sorry for wasting your time, and thanks for your patience at explaining it.