-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
a13e7ba
commit 5a2bb4f
Showing
166 changed files
with
11,785 additions
and
10 deletions.
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
Acidic Sapphire Hedgehog | ||
|
||
Medium | ||
|
||
# Discrepancy in `validateMinAmount` might lead to Rejections when depositing into Vault for instant transaction | ||
|
||
## Summary | ||
The `_validateMinAmount` function exhibits a significant discrepancy between its operational logic and the descriptions provided in the inline comments. This inconsistency has critical ramifications for the _calcAndValidateDeposit function, potentially leading to erroneous validation of deposits. The issue undermines the accuracy of deposit processing and could lead to substantial financial and operational issues. | ||
https://github.com/sherlock-audit/2024-08-midas-minter-redeemer/blob/main/midas-contracts%2Fcontracts%2FDepositVault.sol#L282 | ||
|
||
## Vulnerability Detail | ||
The `_validateMinAmount ` function is intended to enforce minimum deposit requirements by validating both token amounts and their corresponding USD values. According to the inline comments, the function should ensure that the deposit amount in USD is greater than or equal to `minAmountToDepositInUsd()`, and that the token amount is at least `minAmount().` Furthermore, for first-time depositors, the function should validate against `minMTokenAmountForFirstDeposit.` | ||
```solidity | ||
/** | ||
* @dev Validates that inputted USD amount >= minAmountToDepositInUsd() | ||
* and amount >= minAmount() | ||
* @param user user address | ||
* @param amountMTokenWithoutFee amount of mToken without fee (decimals 18) | ||
*/ | ||
``` | ||
The `_validateMinAmoun`t function fails to incorporate USD-based validation, focusing solely on the token amount (amountMTokenWithoutFee). The function does not convert the token amount to its USD equivalent or check it against `minAmountToDepositInUsd().` | ||
|
||
> Consider a situation where a newly onboarded user is attempting their first deposit into a smart contract. The contract stipulates a minimum token quantity requirement for first-time deposits, set at 150,000 XYZ tokens, as defined by the parameter `minMTokenAmountForFirstDeposit.` This parameter dictates that to successfully process their inaugural deposit, a user must deposit at least 150,000 XYZ tokens, regardless of the token’s USD value. In this scenario, the user deposits 200,000 XYZ tokens, easily surpassing the minimum token threshold. However, if the contract lacks additional validation to check the USD value of the deposit, a critical issue arises: the USD value of 200,000 XYZ tokens, at a price of $0.0001 per token, equates to only $20. This amount is substantially below the required USD minimum of $50, which should ideally be enforced to ensure the deposit meets the financial standards set by the contract. | ||
The absence of this USD validation means that while the deposit exceeds the token quantity requirement, it fails to satisfy the necessary USD value, potentially allowing the user to exploit the system by depositing a large volume of low-value tokens. | ||
|
||
This oversight can lead to erroneous deposit approvals or rejections. For example, a deposit of tokens that meets the token minimum but falls short in USD value could be improperly accepted,. | ||
|
||
The discrepancy between the `_validateMinAmount` function’s implementation and its documented intent has significant implications for the `_calcAndValidateDeposit` function. | ||
|
||
## Impact | ||
Incorrect Deposit Approval or Rejection | ||
|
||
## Code Snippet | ||
```solidity | ||
function _validateMinAmount(address user, uint256 amountMTokenWithoutFee) | ||
internal | ||
view | ||
{ | ||
require(amountMTokenWithoutFee >= minAmount, "DV: mToken amount < min"); | ||
if (totalMinted[user] != 0) return; | ||
require( | ||
amountMTokenWithoutFee >= minMTokenAmountForFirstDeposit, | ||
"DV: mint amount < min" | ||
); | ||
} | ||
``` | ||
## Tool used | ||
GitHub | ||
|
||
## Recommendation | ||
Modify _validateMinAmount to include a check against minAmountToDepositInUsd(). Implement a mechanism to convert the token amount to its USD equivalent and ensure it meets the required USD threshold. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,172 @@ | ||
Acidic Sapphire Hedgehog | ||
|
||
Medium | ||
|
||
# First Time Deposit bypassed allowing user to have amountMWithoutFee <= minMTokenAmountForFirstDeposit | ||
|
||
## Summary | ||
The `depositInstant` function in the contract fails to accurately identify first-time depositors due to an improper sequence of operations. Specifically, the incrementing of totalMinted[user] occurs before the validation logic intended to enforce stricter rules for first-time depositors. This logical flaw enables users to bypass the intended minimum deposit requirement for their first transaction, leading to potential exploitation and financial inconsistencies within the system. | ||
https://github.com/sherlock-audit/2024-08-midas-minter-redeemer/blob/main/midas-contracts%2Fcontracts%2FDepositVault.sol#L288-L293 | ||
|
||
## Vulnerability Detail | ||
The `depositInstant` function is designed to process user deposits while ensuring that first-time depositors meet a specified minimum deposit amount (`minMTokenAmountForFirstDeposit`). However, the current implementation of this function incorrectly increments `totalMinted[user]` before invoking the `_validateMinAmount` function, which checks whether the user is a first-time depositor. The increment operation alters the condition used to identify first-time depositors, causing the subsequent validation to fail in applying the stricter minimum deposit requirement. | ||
|
||
The flawed logic can be observed in the following code snippet: | ||
```solidity | ||
function depositInstant( | ||
address tokenIn, | ||
uint256 amountToken, | ||
uint256 minReceiveAmount, | ||
bytes32 referrerId | ||
) | ||
external | ||
whenFnNotPaused(this.depositInstant.selector) | ||
onlyGreenlisted(msg.sender) | ||
onlyNotBlacklisted(msg.sender) | ||
onlyNotSanctioned(msg.sender) | ||
{ | ||
address user = msg.sender; | ||
address tokenInCopy = tokenIn; | ||
uint256 amountTokenCopy = amountToken; | ||
( | ||
uint256 tokenAmountInUsd, | ||
uint256 feeTokenAmount, | ||
uint256 amountTokenWithoutFee, | ||
uint256 mintAmount, | ||
, | ||
, | ||
uint256 tokenDecimals | ||
) = _calcAndValidateDeposit(user, tokenInCopy, amountTokenCopy, true); | ||
require( | ||
mintAmount >= minReceiveAmount, | ||
"DV: minReceiveAmount > actual" | ||
); | ||
totalMinted[user] += mintAmount; <----------- First time total Mint of a user already incremented | ||
_requireAndUpdateLimit(mintAmount); | ||
_tokenTransferFromUser( | ||
tokenInCopy, | ||
tokensReceiver, | ||
amountTokenWithoutFee, | ||
tokenDecimals | ||
); | ||
if (feeTokenAmount > 0) | ||
_tokenTransferFromUser( | ||
tokenInCopy, | ||
feeReceiver, | ||
feeTokenAmount, | ||
tokenDecimals | ||
); | ||
mToken.mint(user, mintAmount); | ||
bytes32 referrerIdCopy = referrerId; | ||
emit DepositInstant( | ||
user, | ||
tokenInCopy, | ||
tokenAmountInUsd, | ||
amountTokenCopy, | ||
feeTokenAmount, | ||
mintAmount, | ||
referrerIdCopy | ||
); | ||
} | ||
``` | ||
```solidity | ||
function _calcAndValidateDeposit( | ||
address user, | ||
address tokenIn, | ||
uint256 amountToken, | ||
bool isInstant | ||
) | ||
internal | ||
returns ( | ||
uint256 tokenAmountInUsd, | ||
uint256 feeTokenAmount, | ||
uint256 amountTokenWithoutFee, | ||
uint256 mintAmount, | ||
uint256 tokenInRate, | ||
uint256 tokenOutRate, | ||
uint256 tokenDecimals | ||
) | ||
{ | ||
require(amountToken > 0, "DV: invalid amount"); | ||
tokenDecimals = _tokenDecimals(tokenIn); | ||
_requireTokenExists(tokenIn); | ||
(uint256 amountInUsd, uint256 tokenInUSDRate) = _convertTokenToUsd( | ||
tokenIn, | ||
amountToken | ||
); | ||
tokenAmountInUsd = amountInUsd; | ||
tokenInRate = tokenInUSDRate; | ||
address userCopy = user; | ||
_requireAndUpdateAllowance(tokenIn, amountToken); | ||
feeTokenAmount = _truncate( | ||
_getFeeAmount(userCopy, tokenIn, amountToken, isInstant, 0), | ||
tokenDecimals | ||
); | ||
amountTokenWithoutFee = amountToken - feeTokenAmount; | ||
uint256 feeInUsd = (feeTokenAmount * tokenInRate) / 10**18; | ||
(uint256 mTokenAmount, uint256 mTokenRate) = _convertUsdToMToken( | ||
tokenAmountInUsd - feeInUsd | ||
); | ||
mintAmount = mTokenAmount; | ||
tokenOutRate = mTokenRate; | ||
if (!isFreeFromMinAmount[userCopy]) { | ||
_validateMinAmount(userCopy, mintAmount); <------------- | ||
} | ||
require(mintAmount > 0, "DV: invalid mint amount"); | ||
} | ||
``` | ||
```solidity | ||
function _validateMinAmount(address user, uint256 amountMTokenWithoutFee) | ||
internal | ||
view | ||
{ | ||
require(amountMTokenWithoutFee >= minAmount, "DV: mToken amount < min"); | ||
if (totalMinted[user] != 0) return; <---------- This would never hold!!!! | ||
require( | ||
amountMTokenWithoutFee >= minMTokenAmountForFirstDeposit, | ||
"DV: mint amount < min" | ||
); | ||
} | ||
``` | ||
The `depositInstant` function calls `_calcAndValidateDeposit` to process deposits. The `_calcAndValidateDeposit` function, in turn, invokes `_validateMinAmoun`t to ensure that the user's deposit meets certain minimum thresholds, particularly for first-time depositors. The _validateMinAmount function is supposed to enforce a stricter minimum for first-time deposits (`minMTokenAmountForFirstDeposit`). However, due to the sequence of operations, this validation can be bypassed. | ||
|
||
Imagine a user, Alice, is making her first deposit into the system. The platform requires that first-time depositors must deposit at least 100 mTokens, represented by the variable `minMTokenAmountForFirstDeposit. `Alice initiates a deposit of 50 mTokens. | ||
|
||
Here’s how the scenario unfolds: | ||
|
||
*Initial Validation:* When Alice’s transaction is processed, _calcAndValidateDeposit is called to handle the deposit logic. The function calculates various values such as `tokenAmountInUsd`, feeTokenAmount, and mintAmount. | ||
|
||
Increment of Total Minted: Before validating if Alice is a first-time depositor, the system increments her totalMinted balance with the mintAmount. For Alice’s case, this increments her totalMinted to 50 mTokens. | ||
|
||
*Validation for First-Time Deposit:* Now, `_validateMinAmount `is called to check if Alice meets the minimum requirements for first-time deposits. However, because totalMinted[Alice] is now 50 mTokens (instead of 0), the function mistakenly concludes that Alice is not a first-time depositor. As a result, the stricter validation against `minMTokenAmountForFirstDeposit` is bypassed. | ||
|
||
*Approval of the Insufficient Deposit:* Since the system bypassed the first-time depositor check, Alice's deposit of 50 mTokens is approved, even though it should have been rejected because it does not meet the required 100 mTokens for first-time depositors. | ||
## Impact | ||
Any user can bypass First Time check in _validateMinAmount` | ||
## Code Snippet | ||
|
||
## Tool used | ||
Manual Review | ||
|
||
## Recommendation | ||
To remediate this vulnerability, it is imperative to re-order the operations in the depositInstant function so that the `_validateMinAmount` function is called before `totalMinted[user]` is incremented. This ensures that the contract accurately identifies first-time depositors and enforces the `minMTokenAmountForFirstDeposit` requirement |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
Fantastic Punch Scallop | ||
|
||
High | ||
|
||
# Wrong tokens decimals due to precision loss causing protocol to lose fees and users to lose tokens | ||
|
||
## Summary | ||
Users are required to pay a fees when depositing tokens into Midas. The calculation of fees is incorrect. For redeeming tokens, the calculation causes users to redeem lesser tokens. | ||
## Vulnerability Detail | ||
In `DepositVault.sol`, the deposits functions calls `_calcAndValidateDeposit` which is used to calculate the tokens to be minted back to user. | ||
Within that function it calls `_truncate` as feeTokenAmount: | ||
```solidity | ||
function _calcAndValidateDeposit( | ||
address user, | ||
address tokenIn, | ||
uint256 amountToken, | ||
bool isInstant | ||
) | ||
internal | ||
returns ( | ||
uint256 tokenAmountInUsd, | ||
uint256 feeTokenAmount, //@audit amount returned is 18 decimals | ||
uint256 amountTokenWithoutFee, | ||
uint256 mintAmount, | ||
uint256 tokenInRate, | ||
uint256 tokenOutRate, | ||
uint256 tokenDecimals | ||
) | ||
-- SNIP -- | ||
feeTokenAmount = _truncate( | ||
_getFeeAmount(userCopy, tokenIn, amountToken, isInstant, 0), | ||
tokenDecimals | ||
); | ||
-- SNIP -- | ||
``` | ||
|
||
In both `DepositVault.sol::depositInstant` and `DepositVault.sol::depositRequest`, the checks for token amounts will cause a revert if the user only approves the required amount of tokens. Thus, users depositing tokens with fewer than 18 decimals will always face a revert issue, because `feeTokenAmount` from the code snippet above returns 1e18 decimals. | ||
|
||
```solidity | ||
function _truncate(uint256 value, uint256 decimals) | ||
internal | ||
pure | ||
returns (uint256) | ||
{ | ||
return value.convertFromBase18(decimals).convertToBase18(decimals); | ||
} | ||
``` | ||
|
||
The problem with `_truncate` is it converts the arguments to 18 decimals. This causes the protocol to earn lesser fees. | ||
|
||
Example Scenario: | ||
Within _getFeeAmount it returns the following `(amount * feePercent) / ONE_HUNDRED_PERCENT;`, assuming the feePercent is 100%, this means the fee to be paid is 100e18. Now due to `_truncate` the fee is converted to 1e18. This causes the protocol to not earn 100% of the deposit fee but 1% instead because of precision loss. | ||
|
||
## Impact | ||
The protocol earns lesser revenue for fees.. | ||
## Code Snippet | ||
https://github.com/sherlock-audit/2024-08-midas-minter-redeemer/blob/main/midas-contracts/contracts/DepositVault.sol#L379 | ||
|
||
## Tool used | ||
|
||
Manual Review | ||
|
||
## Recommendation | ||
Remove the truncate for fees to ensure fees earned is accurate. |
Oops, something went wrong.