sherlock-audit / 2024-05-midas-judging

13 stars 6 forks source link

bhilare_ - A user can bypass minimum deposit requirement, which breaks protocol's requirement/functionality. #124

Closed sherlock-admin3 closed 6 months ago

sherlock-admin3 commented 6 months ago

bhilare_

high

A user can bypass minimum deposit requirement, which breaks protocol's requirement/functionality.

Summary

A user can bypass min deposit requirement, which is a protocol requirement/functionality.

Vulnerability Detail

While depositing a tokenIn using DepositVault::deposit function :

if (!isFreeFromMinDeposit[user]) {
            _validateAmountUsdIn(user, amountUsdIn);
        }

It is being checked , whether the depositer is free from minimum deposit or not. If not, then it being validated whether it is more than the minimum deposit amount. And after that , the user is being set as free from minimum deposit, by setting isFreeFromMinDeposit[user] as TRUE, it is only being done by the vault admin ,using the following function :-

function freeFromMinDeposit(address user) external onlyVaultAdmin {
        require(!isFreeFromMinDeposit[user], "DV: already free");

        isFreeFromMinDeposit[user] = true;

        emit FreeFromMinDeposit(user);
    }

After being free from minimum deposit, a user can deposit any amount of tokens, no restrictions.

But the issue is, while redeeming the token using RedeemptionVault::redeem function, if a user redeems all of his tokens, the boolean isFreeFromMinDeposit[user] is not being set to FALSE again, which means after redeeming all of his tokens, he becomes similar to a first depositer, but can now bypass the minimum deposit requirement and can deposit any little amount of tokens, which break the protocol's requirement/functionality.

So lets say a user deposits 100k EUR amount and then redeems all of it, and then deposits very small amount, he would be able to deposit it, which is not expected by the protocol.

Impact

I did ask sponsor regarding the impact they are expecting, but I only got reply that, It is a protocol's requirement.

So considering this point, submitted this issue , as it breaks the protocol's requirement/functionality.

Code Snippet

https://github.com/sherlock-audit/2024-05-midas/blob/a4a3cc23bb891913ce44665a4cdea9f5c1190f6c/midas-contracts/contracts/DepositVault.sol#L84C1-L112C6

https://github.com/sherlock-audit/2024-05-midas/blob/a4a3cc23bb891913ce44665a4cdea9f5c1190f6c/midas-contracts/contracts/RedemptionVault.sol#L61C5-L77C6

Tool used

Manual Review

Recommendation

First of all, there's no storage being done, on how much a user is depositing.

Hence first there should be a mapping depositedAmount (address ->uint256) , which stores the total amount deposited by the user. And it should be updated accordingly, while depositing and redeeming the tokens.

Then a check should be added while redeeming tokens, whether depositedAmount after the current redeeming will be equal to 0 or not ,i.e the user has redeemed all of his deposited tokens or not. If yes, then the isFreeFromMinDeposit[user] boolean should be set to FALSE.

So that if user deposits again, he would be treated as a first depositer only, and would need to deposit the minimum amount, and hence it would prevent a user from depositing only small amounts.

And hence would prevent from breaking the protocol's requirement/functionality.

Duplicate of #22