`_checkBalances` FUNCTION BREAKS THE EXPECTED BEHAVIOUR OF THE PROTOCOL DUE TO ERRORNEOUS CONDITIONAL CHECK

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L812-L813 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L155-L157 https://github.com/code-423n4/2023-08-shell/blob/main/src/proteus/EvolvingProteus.sol#L149-L151

Vulnerability details

Impact

The EvolvingProteus._checkBalances function is used to verify the token reserve balances and the token reserve ratio are with in the valid boundaries of the pool. To validate the reserve ratio is within the valid range of MIN_M - MAX_M the following checks are performed.

    if (finalBalanceRatio < MIN_M) revert BoundaryError(x,y);
    else if (MAX_M <= finalBalanceRatio) revert BoundaryError(x,y);

The NATSPEC comments for the MIN_M and MAX_M are given as follows:

MIN_M -> This limits the pool to having at most 10**8 x for each y
MAX_M -> This limits the pool to having at most 10**8 y for each x

Even though the documentation says the y / x ratio can have at most 108 the logic implementation reverts when the `y / x == 108` as shown below:

    else if (MAX_M <= finalBalanceRatio) revert BoundaryError(x,y);

But it is not the case with the MIN_M. Because the transaction will not revert when y / x == 1 / 10**8. Hence there is a discrepancy between the documentation and the logic implementation.

Consider a scenario where the y balance = 200 * 10**20 and x balance = 200 * 10**12. Now the y / x = 10**8. The transaction is not expected to be reverted since the documentation mentions that the pool is allowed to have at most 10**8 y for each x. But according to the logic implementation this transaction will revert since MAX_M == finalBalanceRatio. This breaks the expected behaviour of the protocol.

Proof of Concept

        if (finalBalanceRatio < MIN_M) revert BoundaryError(x,y);
        else if (MAX_M <= finalBalanceRatio) revert BoundaryError(x,y);

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

     This limits the pool to having at most 10**8 y for each x.
    */ 
    int128 constant MAX_M = 0x5f5e1000000000000000000;

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

     When a token has 18 decimals, this is one microtoken
    */ 
    int256 constant MIN_BALANCE = 10**12;

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

Tools Used

VSCode and Manual Review

Recommended Mitigation Steps

Hence it is recommended to modify the MAX_M <= finalBalanceRatio conditional check of the _checkBalances function as shown below (omit the equality check in it):

    else if (MAX_M < finalBalanceRatio) revert BoundaryError(x,y);

Assessed type

Other

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[0xRobocop]

May be QA.

Description fails to give impact rather than just "code not as comments".

[viraj124]

that's a typo in the comment we think the severity should be low/informational

[c4-sponsor]

viraj124 marked the issue as disagree with severity

[c4-sponsor]

ishaansinghal (sponsor) confirmed

[c4-judge]

JustDravee changed the severity to QA (Quality Assurance)

[c4-judge]

JustDravee marked the issue as grade-c

[JustDravee]

Merging with https://github.com/code-423n4/2023-08-shell-findings/issues/247 and accepting as typo in comment

[c4-judge]

JustDravee marked the issue as grade-a