Existing checks with `INT_MAX` are insufficient such that the contract becomes dysfunctional after initial deployment of some large balance(s)

[code423n4]

Lines of code

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

Vulnerability details

Impact

EvolvingProteus.sol contains a variety of functions which detail the price in tokens to be paid in swaps, withdraws, and deposits. In external functions such as depositGivenInputAmount(), as well as internal functions such as _check_Balances(), there exists a large number of checks to ensure proper functionality of the contract (during initial deployment and normal operations). For example, most of the external functions check that the amounts of each tokens are smaller than int256 MAX, while _checkBalances() checks that the reserve tokens satisfy a balance ratio range. The coverage is substantial. However, there is one case where overflow may still be possible such that an initial depositer may not be able to get their funds back.

Users who provide an initial deposit large enough balance of one or two of the reserve tokens may be unable to withdraw those tokens (thus being locked in the contract) due to arithmatic overflow (not during initial deployment but rather in subsequent operations).

POC

EvolvingProteus.sol supports 3 different functionalities: swap, deposit, and withdraw, each with two functions. Additionally, all of these functions call _getUtility() at some point in their execution.

Currently, the checks placed on the input parameters of the functions only determine whether or not the parameters are less than the max uint256 value. However, during _getUtility(), many of these parameters are multiplied together, hence potentially exceeding the max uint256 value even if the previous checks were satisfied.

This becomes a problem under the assumption that EvolvingProteus.sol will be used as an oracle for a smart contract that handles monetary currency. In the event that an initial deposit (i.e. the deployer) contains large enough values such that multiplying the currencies in subsequent operations exceeds the max value, the withdraw oracles will always revert, causing the currency to be locked within the contract.

To demonstrate, we provide a runnable test case. Copy the following code into the EvolvingProteus.t.sol file within the test directory and call forge test or forge test --match-contract EvolvingProteusProperties --match-test testBadInput to run.

Code:

function testBadInput() public{
        uint256 MAX_INT = 10**64;
        uint256 x0 = MAX_INT - 1;
        uint256 y0 = MAX_INT - 1;

        uint256 s0 = 10**10;

        uint256 withdrawnAmount = x0/100 * 5;
        SpecifiedToken withdrawnToken = SpecifiedToken.X;

        DUT.withdrawGivenOutputAmount(
                x0,
                y0,
                s0,
                withdrawnAmount,
                withdrawnToken
            );
    }

Expected output and explanation

The test, which simulates a withdraw from the scenario mentioned above, should fail due to [FAIL. Reason: Arithmetic over/underflow]. This is problematic, since the withdraw will revert, hence there is no other way for the currency to be withdrawn.

Recommended Mitigation

Make checks on the deployment function such that _getUtility() cannot overflow under any circumstance (i.e. x*y < INT_MAX) or implement an emergency withdraw function that can only be called by the owners of the smart contract to rescue the funds:

... is Ownable ...

function emergencyWithdraw() onlyOwner external view returns (uint256 withdrawAmt){
    ...
}

Assessed type

Math

[c4-pre-sort]

0xRobocop marked the issue as low quality report

[0xRobocop]

Insufficient proof.

[c4-sponsor]

viraj124 (sponsor) disputed

[c4-judge]

JustDravee marked the issue as unsatisfactory: Insufficient proof