testAssertUint128 validates wrong failure path instead of uint128 overflow

[BZAghalarov]

Hi,

In the testAssertUint128 author checks ExceedsMaxDeposit and ExceedsMaxRedeem conditions.

Due to at the end of vault.deposit(amount, randomUser, self); (also vault.redeem) call priceAssetPerShare.isZero() return True
we get ExceedsMaxDeposit error.

But actually its not intended check, if you change the amount to any random(not more than MAX_UINT128) number this test(attached test function) still will be pass but doesn't check overflow condition.

The deposit function comparison returns true for the following equation
require(assets <= _maxDeposit(vault_, controller), ExceedsMaxDeposit());
because _maxDeposit returns "0"

But actually here when we set vm.assume(amount > 0 && amount < MAX_UINT128);
condition assets variable doesn't go over MAXUINT128. but still we get ExceedsMaxDeposit.

Probably you should remove those 2 checks to the different test function and exact scenario when we get Uint128_Overflow from deposit/redeem call.

P.S I’m not sure whether it would be appropriate to create a PR myself as SR, so I chose not to open one.

function testAssertExceedsMax(uint256 amount) public {
    vm.assume(amount > 0 && amount < MAX_UINT128); // amount has to overflow UINT128
    (, address vault_, ) = deploySimpleVault(VaultKind.Async);
    AsyncVault vault = AsyncVault(vault_);

    vm.expectRevert(IAsyncRequestManager.ExceedsMaxDeposit.selector);
    vault.deposit(amount, randomUser, self);

    vm.expectRevert(IAsyncRequestManager.ExceedsMaxRedeem.selector);
    vault.redeem(amount, randomUser, self);
}

[lemunozm]

Thanks @BZAghalarov for highlighting this. We already have a pending refactor to reorganize spoke tests. Right now, some unit cases like this are computed as part of the integration suite when they should be isolated. We'll fix this as part of that process 👍🏻