KEY INVARIANT RELATED TO THE `FIXED_FEE` AMOUNT CAN BE BROKEN

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L831-L834 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L843-L847

Vulnerability details

Impact

The EvolvingProteus._applyFeeByRounding function is used to charge fees by rounding values in directions that are beneficial to the pool. Within this function there is a if condition which verifies that the calculated final amount is not less than the FIXED_FEE amount as shown below:

if (absoluteValue < FIXED_FEE * 2) revert AmountError();

But this conditional check alone is not enough to verify that the final amount is not less than teh FIXED_FEE amount.

Please consider the following scenario:

1. When the feeUp == false the final amount (roundedAbsoluteAmount) is calculated as follows:

   roundedAbsoluteAmount =
       absoluteValue -
       (absoluteValue / BASE_FEE) -
       FIXED_FEE;

2. There could be a scenario where absoluteValue = FIXED_FEE * 2. If this is the case then the above if statement will not revert (It will only revert if absoluteValue < FIXED_FEE * 2).

3. Assume feeUp == false hence the roundedAbsoluteAmount will be calculated as follows:

   roundedAbsoluteAmount =
       absoluteValue -
       (absoluteValue / BASE_FEE) -
       FIXED_FEE;

4. Now absoluteValue = FIXED_FEE * 2 hence above equation can be modified as follows:

   roundedAbsoluteAmount =
       FIXED_FEE * 2 -
       (absoluteValue / BASE_FEE) -
       FIXED_FEE;

5. Simplified equation will look as follows:

   roundedAbsoluteAmount =
       FIXED_FEE -
       (absoluteValue / BASE_FEE);

Hence it is clear that the final amount (roundedAbsoluteAmount) is less than the FIXED_FEE value which the protocol wanted to stop from happening. If this condition is considered as an invariant then the invariant is broken.

Proof of Concept

Make the EvolvingProteus._applyFeeByRounding a public function for the purpose of testing and add the following testcase to the EvolvingProteusProperties.t.sol test file.

 function testApplyFeeByRounding() public {

    int256 inputAmount = FIXED_FEE * 2;
    int256 returnedValue = DUT._applyFeeByRounding(inputAmount, false);

    assertLt(uint256(returnedValue), uint256(FIXED_FEE));
}

Above test shows that the returnedValue < FIXED_FEE which breaks the invariant of this function which states that the final amount should not be less than the FIXED_FEE.

Following are the areas of the code affected.

    // FIXED_FEE * 2 because we will possibly deduct the FIXED_FEE from
    // this amount, and we don't want the final amount to be less than
    // the FIXED_FEE.
    if (absoluteValue < FIXED_FEE * 2) revert AmountError();

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L831-L834

    } else
        roundedAbsoluteAmount =
            absoluteValue -
            (absoluteValue / BASE_FEE) -
            FIXED_FEE;

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L843-L847

Tools Used

VSCode and Manual Review

Recommended Mitigation Steps

Hence it is recommended to precalculate the absoluteValue / BASE_FEE value before the if statement and include the resulting value in the if conditional check as follows:

uint256 baseFeeRounding = absoluteValue / BASE_FEE
if (absoluteValue < FIXED_FEE * 2 + baseFeeRounding) revert AmountError();

This will make sure that final amount roundedAbsoluteAmount will not be less than the FIXED_FEE amount. Hence the required invariant will hold.

Assessed type

Math

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[0xRobocop]

It may be QA.

It has the requirement that:

absoluteValue - (absoluteValue / BASE_FEE) < FIXED_FEE * 2

Which needs a too small value for absoluteValue, take into consideration that FIXED_FEE is a nano token.

[c4-pre-sort]

0xRobocop marked the issue as low quality report

[c4-pre-sort]

0xRobocop marked the issue as high quality report

[viraj124]

@0xRobocop we haven't listed this as an invariant https://github.com/code-423n4/2023-08-shell/blob/main/README.md#invariants also the comment

    // FIXED_FEE * 2 because we will possibly deduct the FIXED_FEE from
    // this amount, and we don't want the final amount to be less than
    // the FIXED_FEE.

specifies not breaking the require condition with the input amount whereas the poc and issue mentions the output amount which can be less as it doesn't break anything and on top of that we have balance check with the reserve token so yeah to sum it up I don't think this is an issue so will wait for your reply on the same

[c4-sponsor]

viraj124 (sponsor) disputed

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid