maxDeposit doesn't consider global and user-specific limits

[hats-bug-reporter]

Github username: @@Tri-pathi
Twitter username: --
Submission hash (on-chain): 0x2cee54a47c73f1f92b8e4e77e443e317eec90813c9545ab61cfda5e900d35d9b
Severity: medium

Description:
Description

The maxDeposit() function is required to return the maximum amount of underlying assets that can be deposited, factoring in both global and user-specific limits. According to EIP-4626 specifications, if deposits are disabled (even temporarily), the function MUST return 0.

However, the current implementation fails to properly handle all deposit-blocking conditions, leading to inconsistency where deposit() reverts but maxDeposit() returns a positive value.

Proof of Concept

The core invariant - "if deposit is blocked, maxDeposit() must return zero" - is violated in the current implementation:

    function maxDeposit(address) public view returns (uint256) {
        if (paused()) {
            return 0;
        }
        return type(uint256).max;
    }

    function deposit(uint256 assets, address receiver) public returns (uint256 shares) {
        shares = convertToShares(assets);
        usde.burn(msg.sender, assets);  // External call that can revert
        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);
    }

The deposit() function can revert due to several conditions that are not checked in maxDeposit():

1. Missing BURN_ROLE permissions on the USDE token contract
2. Global pausing of USDE token transfers
3. User blacklisting in the USDE token contract

In all these cases, maxDeposit() incorrectly returns type(uint256).max instead of 0.

Impact

This inconsistency between maxDeposit() and deposit() violates EIP-4626 specifications and can lead to integration issues for protocols relying on maxDeposit() to determine valid deposit amounts.

Recommendation

Update maxDeposit() to account for all conditions that could cause deposit() to revert.

function maxDeposit(address user) public view returns (uint256) {
    if (paused() ||
        !usde.hasRole(BURN_ROLE, address(this)) ||
        usde.paused() ||
        usde.isBlacklisted(user)) {
        return 0;
    }
    return type(uint256).max;
}

This is not exact solution but can give you idea how do you want to design this.

Similar updates should be made in maxMint(), maxWithdraw() and maxRedeem().

[AndreiMVP]

While I do agree maxDeposit is not v sophisticated, I don't think this is a rewardable submission.

The deposit() function can revert due to several conditions that are not checked in maxDeposit():

1. Missing BURN_ROLE permissions on the USDE token contract
2. Global pausing of USDE token transfers
3. User blacklisting in the USDE token contract

Condition 1 is not a problem in practice, no need to for the extra gas cost to check that.
Condition 2 has been mentioned in a previous issue #47.
Condition 3 maybe would be nice to take into account but solving it might not be worth the complexity and extra gas costs in practice.. would have to check both the USDE validator and the blacklisted state. So I'd say this is more to be mentioned in documentation that it's a limited function.

[Tri-pathi]

@AndreiMVP @geaxed

1. The issue is about not considering global and user-specific limits in maxDeposit(), maxMint(), maxRedeem(), and maxWithdraw(). Per EIP-4626, these functions must account for all permanent and temporary changes to avoid integration issues. This inconsistency will lead to protocols relying on incorrect limits
2. The examples (missing BURN_ROLE, paused transfers, blacklisting) were illustrative, not exhaustive. These were just to show that global and user-specific limits aren't considered. maxMint(), maxWithdraw(), and maxRedeem() serve different purposes and require separate implementations
3. maxMint() and maxDeposit() lack accuracy #47 does not describe any issue, problem, or impact. From what I can see, it is proposing that maxMint() and maxDeposit() could be enhanced but why? . Hence, there is no similarity with this integration issue.

All four of these functions are used majorly for integration. The whole idea of these function are reflecting global and user specific limit in the view function without initiating transaction

If by extra gas cost you mean deployment cost, then yes, it is worth implementing user and global-specific limits.

[AndreiMVP]

The protocol does try to follow the ERC4626 interface but it doesn't exactly implement the spec - it doesn't work follow the general logic of how such a vault is implemented. I can't defend defend or attack this fact because I didn't take that design decision myself, but it is kinda clear from the contracts. Thus I can only work towards a reasonable outcome.

I think EIP4626 is an over-engineered standard and we can't have a perfect implementation of its functions according to this protocol's intended functioning.

I understand the spirit of your submission follows this:

the current implementation fails to properly handle all deposit-blocking conditions, leading to inconsistency where deposit() reverts but maxDeposit() returns a positive value.

The examples (missing BURN_ROLE, paused transfers, blacklisting) were illustrative, not exhaustive.

However implementing maxDeposit to be completely consistent with deposit is not only impractical (because of the gas fees for lookups during onchain operations associated with the multiple conditions) but also impossible because the standard doesn't allow such a functionality according to our logic: maxDeposit(depositor) vs deposit(assets, receiver) - how would you check for example the validator state of both the sender and receiver in maxDeposit? Former must not be blacklisted on USDE and latter must be whitelisted on EUI but maxDeposit does not allow checking that exactly. Thus according to the current design of the ERC4626 interface and our protocol, while the intention is good, the idea and recommendation of this submission are impractical and do not add value.

#47 does not describe any issue, problem, or impact. From what I can see, it is proposing that maxMint() and maxDeposit() could be enhanced but why? . Hence, there is no similarity with this integration issue.

You seem to minimize the idea of another submission which we have rewarded, then overstate yours - your submission is about maxDeposit and leniently referencing the other max functions, but its implications prove unreasonable so far.