pkqs90 - Rounding direction for the amount of stablecoin user deposit is incorrect

[sherlock-admin2]

pkqs90

medium

Rounding direction for the amount of stablecoin user deposit is incorrect

Summary

The rounding direction for the amount of stablecoin user deposit is incorrect. This would cause the deposit amount to be slightly larger than what the user actually deposited, which is a loss for the protocol.

Vulnerability Detail

During the deposit process, the user specifies the amountUsdIn (in 18 decimals) that he would like to deposit. This amount of mTBILL is minted to the user in the future.

User should transfer the equivalent amount of stablecoin to the receiver. The issue here is when calculating the amount of stablecoin to be transferred, the rounding direction should be up instead of down.

Take USDC as an example. If a user passes amountUsdIn as 1e12-1, the actual amount of USDC that would be sent is (1e12-1) / 1e12 which would be 0, which means users can get 1e12-1 mTBILL tokens for free.

Notice that though this is a very small amount of money (1e-6 USD), if the number of deposits is large enough, this would become large. Also, from the protocol's perspective, this dust amount of money should be charged to the users, or else it may accumulate in the protocol and reach a non-dust value.

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

    function deposit(address tokenIn, uint256 amountUsdIn)
        external
        onlyGreenlisted(msg.sender)
        whenNotPaused
    {
        address user = msg.sender;

        _requireTokenExists(tokenIn);

        lastRequestId.increment();
        uint256 requestId = lastRequestId.current();

        if (!isFreeFromMinDeposit[user]) {
            _validateAmountUsdIn(user, amountUsdIn);
        }
        require(amountUsdIn > 0, "DV: invalid amount");

        totalDeposited[user] += amountUsdIn;
>       _tokenTransferFromUser(tokenIn, amountUsdIn);

        emit Deposit(requestId, user, tokenIn, amountUsdIn);
    }

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/abstract/ManageableVault.sol#L151C1-L157C6

    function _tokenTransferFromUser(address token, uint256 amount) internal {
        IERC20(token).safeTransferFrom(
            msg.sender,
            tokensReceiver,
>           amount.convertFromBase18(_tokenDecimals(token))
        );
    }

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/libraries/DecimalsCorrectionLibrary.sol#L18C1-L54C6

    function convert(
        uint256 originalAmount,
        uint256 originalDecimals,
        uint256 decidedDecimals
    ) internal pure returns (uint256) {
        if (originalAmount == 0) return 0;
        if (originalDecimals == decidedDecimals) return originalAmount;

        uint256 adjustedAmount;

        if (originalDecimals > decidedDecimals) {
>           adjustedAmount =
>               originalAmount /
>               (10**(originalDecimals - decidedDecimals));
        } else {
            adjustedAmount =
                originalAmount *
                (10**(decidedDecimals - originalDecimals));
        }

        return adjustedAmount;
    }

    /**
     * @dev converts `originalAmount` with decimals 18 into
     * amount with `decidedDecimals`
     * @param originalAmount amount to convert
     * @param decidedDecimals decimals for the output amount
     * @return amount converted amount with `decidedDecimals`
     */
    function convertFromBase18(uint256 originalAmount, uint256 decidedDecimals)
        internal
        pure
        returns (uint256)
    {
        return convert(originalAmount, 18, decidedDecimals);
    }

Impact

The protocol would lose dust amount of mTBILL for each deposit, which may accumulate to be a non-dust value.

Code Snippet

• https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/DepositVault.sol#L91C1-L112C6
• https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/abstract/ManageableVault.sol#L151C1-L157C6
• https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/libraries/DecimalsCorrectionLibrary.sol#L18C1-L54C6

Tool used

Manual review

Recommendation

Round up the user transfer for stablecoin instead of rounding down.

[nevillehuang]

Escalate, I believe this issue is at best low severity.

1. There is a minimum 100_000 EUR a user must commit before he can even perform this rounding issue (so you can do the math how hard and how much funds is required before this starts to accumulate + the whole kyc process required to onboard and offboard)
2. Gas costs on mainnet and arbitrum wouldn't incentivize this attack (gas cost would be greater than 0.0000001 USD)

[sherlock-admin3]

Escalate, I believe this issue is at best low severity.

1. There is a minimum 100_000 EUR a user must commit before he can even perform this rounding issue (so you can do the math how hard and how much funds is required before this starts to accumulate + the whole kyc process required to onboard and offboard)
2. Gas costs on mainnet and arbitrum wouldn't incentivize this attack (gas cost would be greater than 0.0000001 USD)

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.

[Welith]

I disagree with the escalation, as this issue shows that there is an incorrect parsing of decimals no matter the initial cost of the user to actually pull it off. You also have the protocol functionality to remove the initial deposit requirement, which can allow the issue to propagate at earlier stages as well. The idea here is that in the event that the above happens, the whole book keeping of the protocol gets messed up, leading to invalid event emission, false token minting, etc.

[MxAxM]

Escalate, I believe this issue is at best low severity.

1. There is a minimum 100_000 EUR a user must commit before he can even perform this rounding issue (so you can do the math how hard and how much funds is required before this starts to accumulate + the whole kyc process required to onboard and offboard)

2. Gas costs on mainnet and arbitrum wouldn't incentivize this attack (gas cost would be greater than 0.0000001 USD)

1- After first deposit, user would be able to deposit without minimum amount 2- It affects protocol by every deposit transaction

It can lead to loss of dust amount but since it's a very small amount I consider it medium, also it's worth mentioning that a similar issue that was utilizing a batch of transactions to steal dust amount was considered valid a previous sherlock contest: https://github.com/sherlock-audit/2024-04-interest-rate-model-judging/issues/68

[Kalogerone]

A user would need to deposit 10 million times to gain 10 euros of totalDeposited. It's not a profitable attack. Also, by protocol design, the protocol team can accept or decline deposits.

[MihailNikolov04]

The issue should be considered valid! I agree that for one user it will be a non-profitable attack to perform, but you should consider that everyone can do it. Also as @MxAxM said, this kind of dust related issues are considered valid in other sherlock contests!

[CarlosAlegreUr]

A user would need to deposit 10 million times to gain 10 euros of totalDeposited. It's not a profitable attack. Also, by protocol design, the protocol team can accept or decline deposits.

I think this is invalid because the attack vector is clearly unprofitable. Let me elaborate more the numbers @Kalogerone mentioned:

Lets imagine you deposit first is in deposit 0_000_000_999_999_999_999 (separeted by _ so easier counting of decimals).

So you deposit 0_000_000_999_999_999_999 == 0000000999999999999 as amountUsdIn which has 18 decimals but usdc has 6 decimals, so if you are free from the minimum deposit limit you can deposit the 0_000_000_999_999_999_999 which is > 0 and then at the transfer it converts it to 6 decimals for USDC so you pay 0, but the protocol still accounts for 0.000000999999999999, making the protocol account for 0.000000999999999999$ extra and, technically a loss.

BUT. You can only generate a loss of as much 0.000000999999999999$ per deposit. This means that for a loss of simply 1$ you would need 1/0.000000999999999999=1_000_000$. So, to generate 1$ of losses to the protocol the attacker must spend a million, completely unprofitable. Also we are not taking into account gas, which will be extra costs for the attacker.

Due to the insanely unproftability of the attack, I think its invalid or as much, a Low issue. We are talking about an attacker that spends 10^6 times more $ that the amount that the protocol losses.

[blutorque]

Escalate, I believe this issue is at best low severity.

1. There is a minimum 100_000 EUR a user must commit before he can even perform this rounding issue (so you can do the math how hard and how much funds is required before this starts to accumulate + the whole kyc process required to onboard and offboard)

2. Gas costs on mainnet and arbitrum wouldn't incentivize this attack (gas cost would be greater than 0.0000001 USD)

1- After first deposit, user would be able to deposit without minimum amount 2- It affects protocol by every deposit transaction

It can lead to loss of dust amount but since it's a very small amount I consider it medium, also it's worth mentioning that a similar issue that was utilizing a batch of transactions to steal dust amount was considered valid a previous sherlock contest: sherlock-audit/2024-04-interest-rate-model-judging#68

According to sherlock rules, the loss must exceed a small and finite amount to be eligible for a medium, which is not the case here.

[Kirkeelee]

A user would need to deposit 10 million times to gain 10 euros of totalDeposited. It's not a profitable attack. Also, by protocol design, the protocol team can accept or decline deposits.

If you follow the flow that amount is not transferred from the user at all. The issue here is that users get almost free tokens when they call with certain amount. The amount user calls the function is not withdrawn from the user which leads to loss of funds for the protocol. Claim that it is not a profitable attack is incorrect since the protocol is just giving away their tokens for dust amounts. Imagine you are serious user and want to deposit 100k but when the transaction ends your balance has decreased by only one cent. Protocol checks the emissions but not the contract balance and see 100k and mint amount of tokens corresponding to 100k.

[CarlosAlegreUr]

0.000000999999999999$

A user would need to deposit 10 million times to gain 10 euros of totalDeposited. It's not a profitable attack. Also, by protocol design, the protocol team can accept or decline deposits.

If you follow the flow that amount is not transferred from the user at all. The issue here is that users get almost free tokens when they call with certain amount. The amount user calls the function is not withdrawn from the user which leads to loss of funds for the protocol. Claim that it is not a profitable attack is incorrect since the protocol is just giving away their tokens for dust amounts. Imagine you are serious user and want to deposit 100k but when the transaction ends your balance has decreased by only one cent. Protocol checks the emissions but not the contract balance and see 100k and mint amount of tokens corresponding to 100k.

But the max amount someone can get from 1 call to deposit is 0.000000999999999999$ and the gas cost for that, even in the cheaper chain Arbitrum, is higher than 0.000000999999999999$.

The gas cost of the latest txs as of now in arbitrum as of now is around 0.00000000001 ETH. See here.

That means that only 1 unit of gas would cost you 0.00000000001 ETH, ETH is currently around 3.6K dollars. See coinmarketcap eth. Thus just 1 unit of gas would cost you:

0.00000000001 ETH * 3600 $ = 0.00000036$/1 unit of gas

And clearly executing deposit() will cost you more than 1 unit of gas. Still 1 unit of gas in the cheapest supported chain is smaller than the amount of value the attaker can extract from the protocol:

0.000000999999999999$ / 0.000_000_36$ = 2.75

This means that the profitability per call is 2.75 bigger that the price of executing 1 unit of gas, thus if the gas consumed is 2.75 time more than 1 then the attack is unprofitable. But also clearly deposit() will have greater costs than 1 * 2.75 = 2.75 gas. A single add op code is already 3 of gas: https://www.evm.codes/

The attack is super unprofitable even if you transfer 0 tokens due to gas spendings.

[Kirkeelee]

Then what happens when you deposit USDC, which has 6 decimals? Do the calculations, and you will see the difference. Just think of it as normal user operation, not an attempt to attack.

[Welith]

Totally agree with @Kirkeelee , that's what I've shown here https://github.com/sherlock-audit/2024-05-midas-judging/issues/169

[pronobis4]

The math is right, but isn't the main checking of deposits done offchain? :) In my opinion, the fact that this is a mainly off-chain project reduces the importance of this submission.

[MxAxM]

The math is right, but isn't the main checking of deposits done offchain? :) In my opinion, the fact that this is a mainly off-chain project reduces the importance of this submission.

It doesn't have anything with off-chain calculations, problem is protocol rounds down transfer amount

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/RedDuck-Software/midas-contracts/pull/45

[WangSecurity]

As I understand from the submission and discussion, the issue is that the user can deposit 0 USDC and get 1e12-1 mTBILL. But let's look at that from README:

The conversion of USDC deposits to mTBILL tokens and vice-versa is performed offchain. These assets are bankruptcy protected, with attestation reports posted daily on the Midas site. This will be fully functional and working as intended. All reports are independently verified, and there should be no delays in reporting or posting the data.

So how do we know what amount of mTBILL tokens will be minted to the user? Do we know how this off-chain mechanism work (I mean objective evidence, e.g. docs, messages, and not logical assumption)?

[WangSecurity]

Since no answer is provided, I'll make the decision based on the currently available information. It's fair to assume that mTBILL will be minted based on the amount emitted in the event of deposit function. But, it's not guaranteed the protocol will do it that way. Moreover, it's quite easy to check what amount was sent via deposit function.

Hence, I believe this off-chain mechanic will handle this problem of amount emitted in the event and actual amount sent being deifferent.

Planning to accept the escalation and invalidate the issue.

[MxAxM]

I think mTBILL tokens would be minted for users based on totalDeposited mapping otherwise it shouldn't be implemented

What you mentioned (checking what amount was sent via deposit function) can be considered a Mitigation for the issue, since it wasn't listed at known issues I believe it should be considered valid

However, I respect your decision on this

[Welith]

Isn't part of the issue exactly the fact that you can get an invalid event emission based on this discrepancy? If you deposit USDC the event emitted will be totally invalid due to these decimal issues.

[WangSecurity]

@MxAxM It's a very fair point and it's logical, but we can't know for sure how it's actually done, unfortunatelly.

@Welith Yes, emitted event will contain wrong number, but as I've said we can't know how this conversion will work and how this off-chain mechanic will deal with contract balance not increasing while the variable value is higher.

Hence, it's a guess how this will be dealt and considered, despite it fairly being logical.

Therefore, the decision remains the same, accept the escalation and leave the issue as it is.

[Welith]

But in addition to the wrongly emitted event (I agree that the protocol can easily check everything off-chain) doesn't this issue also break the internal bookkeeping? The amount emitted will be added to the totalDeposit as well. There is no way to change this value internally so by allowing this the functionality of the on-chain logic is damaged.

[Kirkeelee]

One point is that if the protocol hits high volumes (thousands of transactions per day), it is not feasible to check the contract balance or the deposit transaction for each user. Given there will also be redemptions.

[WangSecurity]

The amount emitted will be added to the totalDeposit as well. There is no way to change this value internally so by allowing this the functionality of the on-chain logic is damaged

What is impact of this on the workflow of the protocol, despite that it might effect off-chain conversion?

One point is that if the protocol hits high volumes (thousands of transactions per day), it is not feasible to check the contract balance or the deposit transaction for each user. Given there will also be redemptions.

Fair point, but again, we don't know how the off-chain mechanic for conversion works and how the protocol will verify the amounts deposited.

Hence, the decision remains the same, accept the escalation and invalidate the issue.

[Welith]

I am quite new to the escalation part of the audits, thus the questions. However, It's just a bit strange that the protocol allows for their product to produce wrong results, save these results on-chain, currently not being able to be changed by any means. I just can't see this issue being an intentional design. I know there is a lot of off-chain here, but aren't we supposed to judge the on-chain one more?

[zyppyvids]

@WangSecurity Also, if they handled all of this off-chain why even bother to confirm and fix this issue? As far as I understand the whole idea of the sponsors confirming an issue is to show that they actually think it's a valid one, isn't this true?

[WangSecurity]

However, It's just a bit strange that the protocol allows for their product to produce wrong results, save these results on-chain, currently not being able to be changed by any means. I just can't see this issue being an intentional design.

I didn’t say it’s intentional design, but I asked how totalDeposit impacts the protocol, besides that it might be used for off-chain mechanic. If it’s wrong but it doesn’t break the functionality of the contract in any way, then I see it as a low severity (note, I don’t say it’s wrong, I say that if it doesn’t have a clear impact on the contract’s functionality, then it’s low severity).

Also, if they handled all of this off-chain why even bother to confirm and fix this issue? As far as I understand the whole idea of the sponsors confirming an issue is to show that they actually think it's a valid one, isn't this true?

Fair point, but the fact that sponsor confirms and fixes the issue doesn’t affect the final judgement.

The decision remains the same, planning to accept the escalation and invalidate the report

[Welith]

@WangSecurity just for future references, in your view what is needed for an issue like this to be validated? I am not trying to argue or anything, basically smart contracts with an apparent error, that do not have enough sponsor documentation and clarification around the error, should not be reported?

[Welith]

In addition to my above question, my query is coming from the fact that I've spent a decent amount of time proving my point in here showing with code that the on-chain logic is flawed, same as the author of this issue, but it gets invalidated because we are focusing a lot on off chain handling.

[Tri-pathi]

@WangSecurity

There are only two variables from which offchain mechanism can know that what is the deposited value and both will create issue since they are not actual transfer amount

There is only way it can be handled offchain , which is taking the emitted amount and again round down in the same manner to recalculate what was actual transfer amount.

Which sounds like mitigation of above issue, which has root cause in inscope smart contract. It doesn't matter to SRs whether mitigation is offchain or onchain,it depends on the protocol team.

Or let me know if there is rule which says that inscope issue will be invalid if the mitigation can be done offchain ?

[Welith]

@Tri-pathi great point! As I am now looking at Wang's main argument is that we are not sure if this error might affect the protocol on-chain, but we are not sure if they might do something about it off-chain. So basically the accepted escalation is more in favour of the off-chain probability, which is kinda strange for a on-chain competition. We have proof of an on-chain error, whereas we have nothing on the off-chain.

[WangSecurity]

Firstly, I didn't say the vulnerability is invalid, in fact, I agree it's valid. But, the impact of this is that it may have affect on the mTBILL minting and conversion, but there's no concrete evidence if it indeed effects off-chain in any way. About these two variables being the only ones to get the data off-chain and mint tokens, I believe I've already repeated a couple of times above, why this argument doesn't make it H/M.

For this issue to be valid in that case, there either should be no off-chain mechanisms and everything would be done on-chain OR it has to have M/H impact on-chain. Currently, the only impact it has on chain is inflated totalDeposit variable, but as I understand it doesn't lead to any loss of funds or renders contract useless.

Decision remains the same, accept the escalation and invalidate the report.

[Welith]

But doesn't it kinda make the protocol functionality useless, as if I make the inflated deposit my data in the contract in terms of total deposited will be corrupted, with no way of being fixed without me having to either create a new account or issue for a chargeback and never use the protocol again? Kinda like a DoS as the protocol will most probably block my account even though I wouldn't have known this issue existed and I deposited USDC.

[WangSecurity]

“The protocol will most probably block my account”, excuse me, but it seems like an assumption and speculation, and I don’t see how it changes anything. Moreover, with Trusted admins it’s not an issue or an argument.

As I’ve said, there is no on-chain H/M impact that would be caused by this inflated variable. I agree it’s not good, but it doesn’t qualify for the medium severity.

The decision remains the same, accept the escalation and invalidate the report.

[WangSecurity]

Result: Invalid Has duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nevillehuang: accepted

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.