Missing balance checks in `_reserveTokenSpecified()`

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L563-L595 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L701-L724 https://github.com/code-423n4/2023-08-shell/blob/c61cf0e01bada04c3d6055acb81f61955ed600aa/src/proteus/EvolvingProteus.sol#L716

Vulnerability details

Impact

There is a missing balance check in _reserveTokenSpecified(), which allows reserve tokens to be withdrawn or deposited into illegal balances, which may cause subsequent transactions to revert. It also allows the pool to be killed by withdrawing all balances which prevents any further deposit.

Proof of Concept

The allowed region of possible reserve token balances is constrained by _checkBalances(), which requires that each balance be at least MIN_BALANCE and that the ratio of balances be in the interval [MIN_M, MAX_M).

The functions available for swapping are swapGivenInputAmount() and swapGivenOutputAmount(), both of which make use of _swap() for calculating the unknown value (output and input amounts) which in turn checks the resulting balance after the swap with _checkBalances(). Thus it is impossible to swap out of the allowed region.

The functions depositGivenOutputAmount() and withdrawGivenInputAmount() both make use of _lpTokenSpecified() for the calculation which likewise checks the resulting balance after the deposit or withdrawal with _checkBalances(). Thus it is impossible to deposit or withdraw, using these functions, out of the allowed region.

However, the remaining functions depositGivenInputAmount() and withdrawGivenOutputAmount() instead make use of _reserveTokenSpecified() which fails to perform the balance checks. The balances may therefore end up outside of the allowed region when using these functions. Specifically, one may withdraw too much such that MIN_BALANCE is violated or one may deposit too much such that the ratio of balances is violated.

By entering this forbidden region, some transactions will be reverted if they fail to leave the forbidden region. It also makes it possible to kill the pool by withdrawing all of both reserve tokens. Then all deposits will revert due to division by zero in _reserveTokenSpecified() at L589 and in _getUtilityFinalLp() at L660 (called by _lpTokenSpecified()).

Also note that _getUtility() accepts negative balances as parameters. This means (depending on the integration of EvolvingProteus) that one can withdraw into a negative balance. Further note the unsafe casting of an integer into an uint256 at L716 in the calculation of disc. This is an issue precisely when we have this freedom with the balances. The utility may thus underflow into an invalid and much higher value, breaking an important invariant.

Recommended Mitigation Steps

Add balance checks in _reserveTokenSpecified(), as in _swap() and _lpTokenSpecified().

Consider also checking that x and y in _getUtility() are non-negative and that the uint256 casting in the calculation of disc doesn't underflow.

Assessed type

Invalid Validation

[c4-pre-sort]

0xRobocop marked the issue as primary issue

[c4-pre-sort]

0xRobocop marked the issue as sufficient quality report

[c4-sponsor]

viraj124 marked the issue as disagree with severity

[viraj124]

this should be low/med at best IMO we're adding the balance check but note _getUtility is a internal method and there are input check of the reserve balances values passed so that is a invalid argument

[c4-sponsor]

viraj124 (sponsor) acknowledged

[viraj124]

we checked this further with some other members of the team and agreed this is a high and we've fixed this in a pr

[c4-sponsor]

viraj124 (sponsor) confirmed

[c4-judge]

JustDravee marked the issue as satisfactory

[c4-judge]

JustDravee marked issue #57 as primary and marked this issue as a duplicate of 57