enfrasico - Deposits cannot be made when there is no limit for totalSupply and per wallet limit

[sherlock-admin2]

enfrasico

medium

Deposits cannot be made when there is no limit for totalSupply and per wallet limit

Summary

Deposits cannot be made due to overflow when tsLimit == uint256.max && walletLimit == uint256.max as _maxMint returns uint256.max under these conditions.

Vulnerability Detail

The code flow will look like this when a user tries to deposit.

1. deposit() is called
2. It calls maxDeposit() to check if the assets currently deposited is more than what the user can deposit at maximum
3. maxDeposit() calls convertToAssets() which calls _maxMint()
4. _maxMint() returns uint256.max when tsLimit == uint256.max && walletLimit == uint256.max
5. Overflow happens at convertToAssets() since uint256.max cannot be multiplied with any number > 0

Impact

Deposits cannot be made by users when we want to set no limits for total supply and per wallet supply.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L936-L938

Tool used

Manual Review

Recommendation

Consider this special case for when we want do not want to restrict total supply and per wallet supply.

Duplicate of #577

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

Trumpero commented:

low, totalSupplyLimit and perWalletLimit will be set by the admin (in the constructor and setter function). The deposit() function is disabled until these parameters are set, and it does not cause any loss

[yixxas]

Escalate

This is a valid medium. This is a bug in core functionality. It is true that totalSupplyLimit and perWalletLimit are set by the admin, but it is part of the Tokemak's specification that these values are set to uint256.max if the protocol wants to allow unlimited supply or unlimited wallet. But setting these values to such will prevent deposit() from being callable and hence users cannot mint shares with deposit().

[sherlock-admin2]

Escalate

This is a valid medium. This is a bug in core functionality. It is true that totalSupplyLimit and perWalletLimit are set by the admin, but it is part of the Tokemak's specification that these values are set to uint256.max if the protocol wants to allow unlimited supply or unlimited wallet. But setting these values to such will prevent deposit() from being callable and hence users cannot mint shares with deposit().

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[nevillehuang]

Escalate

#465, #503 and #731 are duplicates of this issue

[xiaoming9090]

This issue should be valid and fixed. An overflow could occur if the admin decided to set it to "no limit".

[sherlock-admin2]

Escalate

#465, #503 and #731 are duplicates of this issue

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Trumpero]

I understand that this is a valid issue and needs to be fixed, but I believe it should be a low issue. It doesn't cause any loss of funds since users are unable to deposit any funds in this case. It can't break the protocol, as the admin is able to set a new configuration and resolve it (they can config a very large value for perWalletLimit, such as type(uint128).max, instead of type(uint256).max). Therefore, I believe the impact of this issue is low and doesn't meet the requirements of a medium issue.

[Evert0x]

I believe this issue falls into the following category

https://docs.sherlock.xyz/audits/judging/judging#vii.-list-of-issue-categories-that-are-not-considered-valid

Issues that cause minor inconvenience to users where there is no material loss of funds are not considered valid. Funds are temporarily stuck and can be recovered by the administrator or owner. Also, if a submission is a design opinion/suggestion without any clear indications of loss of funds is not a valid issue.

Planning to reject escalation and keep invalid

[Evert0x]

Result:
Invalid
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• yixxas: rejected
• nevillehuang: rejected

[yixxas]

Hi @Evert0x, I believe there is a mistake here, and would like to request for a re-review. According to Trumpero's comment, and I quote, "admin is able to set a new configuration and resolve it (they can config a very large value for perWalletLimit, such as type(uint128).max", but this directly contradicts the specification of ERC-4626.

If we look at ERC4626's specification for maxDeposit,

Specifically, if there is no limit for the maximum amount of assets to be deposited, the vault MUST return 2**256 - 1. The protocol cannot simply set the value to any other value since Tokemak's vault is intended to be integrated with other protocols. Furthermore, it is part of the contest's README that explicitly requires the vault to be ERC-4626 compliant.