| Audit Date | 11/23/2023 - 11/25/2023 |
| Auditor | HollaDieWaldfee (@HollaWaldfee100) |
| Version 1 | 11/25/2023 Initial Report |
| Version 2 | 11/27/2023 Mitigation Review |
- Audit Report - Mute
- Contents
- Disclaimer
- About HollaDieWaldfee
- Scope
- Centralization Risks
- Severity Classification
- Summary
- Findings
- High Risk Findings (5)
- 1. Wrong calculation of
payoutDecreasevariable inwithdraw()function can trap users and cause loss of funds - 2.
supplyIndex0andsupplyIndex1are incorrectly updated inredeemRewards()function - 3. Wrong calculation of partial rewards allows to bypass
epochDuration - 4. Reward multiplier can be manipulated by making multiple smaller deposits
- 5.
MuteOraclecan be manipulated
- 1. Wrong calculation of
- Medium Risk Findings (3)
- Low Risk Findings (3)
- Improvement Findings (5)
- High Risk Findings (5)
The following smart contract audit report is based on the information and code provided by the client, and any findings or recommendations are made solely on the basis of this information. While the Auditor has exercised due care and skill in conducting the audit, it cannot be guaranteed that all issues have been identified and that there are no undiscovered errors or vulnerabilities in the code.
Furthermore, this report is not an endorsement or certification of the smart contracts, and the Auditor does not assume any responsibility for any losses or damages that may result from the use of the smart contracts, either in their current form or in any modified version thereof.
HollaDieWaldfee is a top ranked Smart Contract Auditor performing audits on code4rena and Sherlock. He has placed 1st in multiple audit contests.
On Sherlock he uses the handle "roguereddwarf" to compete in audit contests.
He can also be booked for conducting Private Audits.
Contact:
Twitter/X: @HollaWaldfee100
The audit has been conducted on the code in the client's GitHub repository (https://github.com/muteio/mute-switch-core) at commit 554dac6bd3f897d47391e7ebe92597c519219eed.
The following source files are in scope:
The mitigation review has been conducted at commit 0732aa76555c613a3ab92b10d9b9113b36019ba8.
The MuteAmplifierRedux contract makes use of a privileged owner role.
He is allowed to set the MuteAmplifierRedux contract's parameters to any value, thereby being able to break the contract, and allowing no further deposits.
He is also privileged to transfer the MUTE reward tokens from the MuteAmplifierTreasury contract. In the case that rewards cannot be paid out, users can still use the MuteAmplifierRedux.emergencyWithdraw() function to withdraw their LP tokens.
In the worst case, the owner can increase the management_fee to 100% shortly before a user makes a deposit. This results in a complete loss of LP tokens for the user.
LP tokens that have been deposited cannot be stolen, they can always be withdrawn by the user.
In summary, the owner must be considered trusted.
Trust assumptions could be lowered by enforcing tighter value ranges for the parameters that the owner can set.
Note that this assessment of the centralization risks only takes into account the MuteAmplifierRedux contract, the system as a whole must also be assessed before interacting with it.
| Severity | Impact: High | Impact: Medium | Impact: Low |
|---|---|---|---|
| Likelihood: High |  |
|
 |
| Likelihood: Medium | |
|
 |
| Likelihood: Low | |
|
|
Impact - the technical, economic and reputation damage of a successful attack
Likelihood - the chance that a particular vulnerability is discovered and exploited
| Severity | Total | Fixed | Partially Fixed | Acknowledged | Disputed | Reported |
|---|---|---|---|---|---|---|
|
5 | 4 | 1 | 0 | 0 | 0 |
|
3 | 3 | 0 | 0 | 0 | 0 |
|
3 | 3 | 0 | 0 | 0 | 0 |
|
5 | 5 | 0 | 0 | 0 | 0 |
1. Wrong calculation of payoutDecrease variable in withdraw() function can trap users and cause loss of funds 
Description:
The payoutDecrease variable in the withdraw() function is incremented by rewardTot (the sum of rewards) in each iteration of the loop over the stakes (Link).
Say the rewards for the different stakes are 10, 11, 12.
The intended result is 10 + 11 + 12 = 33.
However, the actual result is 10 + (10 + 11) + (10 + 11 + 12) = 64.
Impact:
The pending rewards are tracked such that users can only deposit an amount of lpTokens such that there are enough rewards available to be paid out (Link).
By calling withdraw() (either as part of normal operation or as a griefing attack), users will be able to deposit more lpTokens than they should.
This means they may have to pay a management_fee without being able to redeem their rewards (-> loss of funds).
Given that this scenario will most likely occur at some point and is a direct loss of funds to users, this issue is of "High" severity.
Recommendation:
Increment the payoutDecrease variable correctly:
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..9122df1 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -340,7 +340,7 @@ contract MuteAmplifierRedux is ReentrancyGuard {
rewardTot = rewardTot + (remainingRewards * (block.timestamp - _stakes[i].stakedTime) / epochDuration);
}
- payoutDecrease += rewardTot;
+ payoutDecrease += remainingRewards;
}
delete userStakes[msg.sender];Mitigation Review:
This issue has been fixed as recommended.
Description:
Each stake has a supplyIndex0 and supplyIndex1 variable associated with it.
This is needed to calculate the pool fees that a user is supposed to receive for the lpTokens he has deposited into the MuteAmplifierRedux contract.
The issue is that in the redeemRewards() function, the stake indexes are set to the supply indexes of msg.sender (Link).
Other than in the _deposit() function where the stake indexes are correctly set like this (since supply indexes are updated upon lpToken transfers) (Link), this is not correct in the redeemRewards() function.
When the last supply indexes update for msg.sender is in the past, msg.sender will receive more fees for his lpToken deposit than he should the next time fees are calculated, effectively stealing the fees from other users that have not yet redeemed their rewards.
Impact:
A malicious user can deliberately steal fees from other users.
Also, fee accounting will be wrong in regular operation.
Recommendation:
The supply indexes for the stake must be set to the current supply indexes of address(this) which is equal to the global current indexes which are already saved on the stack. So the code can be changed like this:
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..3aff8c6 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -251,8 +251,8 @@ contract MuteAmplifierRedux is ReentrancyGuard {
_stakes[i].rewardClaimed = _stakes[i].rewardClaimed + partialRewards;
- _stakes[i].supplyIndex0 = IMuteSwitchPairDynamic(lpToken).supplyIndex0(msg.sender);
- _stakes[i].supplyIndex1 = IMuteSwitchPairDynamic(lpToken).supplyIndex1(msg.sender);
+ _stakes[i].supplyIndex0 = index0;
+ _stakes[i].supplyIndex1 = index1;
}
}Mitigation Review:
This issue has been fixed as recommended.
In addition, the _deposit() function now queries the supply indexes of address(this) instead of msg.sender (Link). This helps make the code more understandable but it is not strictly necessary since it provides the same result as discussed in the "Description" section.
Description:
Here's how partial rewards are calculated:
(Link)
uint256 partialRewards = (remainingRewards * (block.timestamp - _stakes[i].stakedTime) / epochDuration);
rewardTot = rewardTot + partialRewards;
_stakes[i].rewardClaimed = _stakes[i].rewardClaimed + partialRewards;Let's consider an example:
block.timestamp = 200
_stakes[i].stakedTime = 100
epochDuration = 200
_stakes[i].rewardClaimed = 0
remainingRewards = 100
Based on the above calculations, we get:
partialRewards = 100 * (200 - 100) / 200 = 50
_stakes[i].rewardClaimed = 50
This is correct, 50% of rewards are paid out since 50% of the duration has passed.
But let's consider what happens when redeemRewards() is called a second time:
block.timestamp = 200
_stakes[i].stakedTime = 100
epochDuration = 200
_stakes[i].rewardClaimed = 50
remainingRewards = 100
Based on the above calculations, we get:
partialRewards = 50 * (200 - 100) / 200 = 25
_stakes[i].rewardClaimed = 75
Even though no further time has passed, we were able to claim another 25 rewards.
Only the first calculation of partial rewards is correct.
If a user redeems partial rewards more than one time, the calculation becomes incorrect and he gets more rewards than he should be able to.
Both the redeemRewards() and withdraw() function is affected by this.
Impact:
An attacker can get almost all rewards without waiting the full epochDuration by applying a strategy like the following:
- Make a deposit and wait 10% of the epoch duration
- As long as it is profitable, call
redeemRewards()repeatedly - Then call
withdraw() - Repeat
Thereby an attacker can almost 10x his reward rate
Recommendation:
The changes ensure that the rewards are paid out correctly over time, also epochDuration is now fixed per stake (which is in line with the fact that reward amounts are also fixed and cannot be changed once a stake is created).
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..e433f23 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -36,6 +36,9 @@ contract MuteAmplifierRedux is ReentrancyGuard {
uint256 stakedTime; // unix time of stake
uint256 rewardAmount; // amount of mute to award this stake
uint256 rewardClaimed; // amount claimed so far
+ // epoch duration should be fixed per stake
+ uint256 epochDuration;
+ uint256 lastRedeem;
uint256 supplyIndex0;
uint256 supplyIndex1;
@@ -194,6 +197,8 @@ contract MuteAmplifierRedux is ReentrancyGuard {
block.timestamp,
payout,
0,
+ epochDuration,
+ block.timestamp,
IMuteSwitchPairDynamic(lpToken).supplyIndex0(msg.sender),
IMuteSwitchPairDynamic(lpToken).supplyIndex1(msg.sender)
));
@@ -221,7 +226,7 @@ contract MuteAmplifierRedux is ReentrancyGuard {
IMuteSwitchPairDynamic(lpToken).claimFees();
for(uint256 i = _stakes.length - 1; i >= 0; i--){
- uint256 expiry = _stakes[i].stakedTime + epochDuration;
+ uint256 expiry = _stakes[i].stakedTime + stakes[i].epochDuration;
uint256 remainingRewards = _stakes[i].rewardAmount - _stakes[i].rewardClaimed;
uint256 index0 = IMuteSwitchPairDynamic(lpToken).index0();
@@ -246,10 +251,11 @@ contract MuteAmplifierRedux is ReentrancyGuard {
_stakes[i] = _stakes[_stakes.length - 1];
_stakes.pop();
} else {
- uint256 partialRewards = (remainingRewards * (block.timestamp - _stakes[i].stakedTime) / epochDuration);
+ uint256 partialRewards = (remainingRewards * (block.timestamp - _stakes[i].lastRedeem) / (_stakes[i].stakedTime + stakes[i].epochDuration - _stakes[i].lastRedeem));
rewardTot = rewardTot + partialRewards;
_stakes[i].rewardClaimed = _stakes[i].rewardClaimed + partialRewards;
+ _stakes[i].lastRedeem = block.timestamp;
_stakes[i].supplyIndex0 = IMuteSwitchPairDynamic(lpToken).supplyIndex0(msg.sender);
_stakes[i].supplyIndex1 = IMuteSwitchPairDynamic(lpToken).supplyIndex1(msg.sender);
@@ -285,14 +291,14 @@ contract MuteAmplifierRedux is ReentrancyGuard {
uint256 rewardTot;
for(uint256 i = _stakes.length - 1; i >= 0; i--){
- uint256 expiry = _stakes[i].stakedTime + epochDuration;
+ uint256 expiry = _stakes[i].stakedTime + stakes[i].epochDuration;
uint256 remainingRewards = _stakes[i].rewardAmount - _stakes[i].rewardClaimed;
if(block.timestamp > expiry){
rewardTot = rewardTot + remainingRewards;
} else {
- uint256 partialRewards = (remainingRewards * (block.timestamp - _stakes[i].stakedTime) / epochDuration);
+ uint256 partialRewards = (remainingRewards * (block.timestamp - _stakes[i].lastRedeem) / (_stakes[i].stakedTime + stakes[i].epochDuration - _stakes[i].lastRedeem));
rewardTot = rewardTot + partialRewards;
}
}
@@ -316,7 +322,7 @@ contract MuteAmplifierRedux is ReentrancyGuard {
IMuteSwitchPairDynamic(lpToken).claimFees();
for(uint256 i = _stakes.length - 1; i >= 0; i--){
- uint256 expiry = _stakes[i].stakedTime + epochDuration;
+ uint256 expiry = _stakes[i].stakedTime + stakes[i].epochDuration;
uint256 remainingRewards = _stakes[i].rewardAmount - _stakes[i].rewardClaimed;
uint256 index0 = IMuteSwitchPairDynamic(lpToken).index0();
@@ -337,7 +343,7 @@ contract MuteAmplifierRedux is ReentrancyGuard {
if(block.timestamp > expiry){
rewardTot = rewardTot + remainingRewards;
} else {
- rewardTot = rewardTot + (remainingRewards * (block.timestamp - _stakes[i].stakedTime) / epochDuration);
+ rewardTot = rewardTot + (remainingRewards * (block.timestamp - _stakes[i].lastRedeem) / (_stakes[i].stakedTime + stakes[i].epochDuration - _stakes[i].lastRedeem));
}
payoutDecrease += rewardTot;Mitigation Review:
This issue has been fixed as recommended.
Description:
Note: This issue has been found by the client while preparing the fixes for the other issues.
The reward multiplier depends on the value of LP token to ve shares (Link).
This can be exploited as follows: If a user wants to deposit $10k in LP token and holds $5k in ve shares, he can split the $10k deposit into two $5k deposits to still get the full multiplier.
Impact
Ease of exploitation and the profit that an attacker can gain make this a High severity issue.
Mitigation Review
The issue has been fixed by taking into account the total LP token stake when calculating the multiplier (Link).
It is now still possible to earn higher rewards by splitting a big LP token deposit into smaller deposits.
Assume a $10k deposit and $5k ve shares.
If the $10k is deposited in one deposit, the multiplier will be $5k / $10k = 50%.
If the $10k is deposited in two deposits, the multiplier for the first deposit will be $5k / $5k = 100% and for the second deposit it will be $5k / $10k = 50%. This makes the average 75%.
We see that it's still favored to make multiple smaller deposits. However, while the average multiplier is still higher than it would be ideally, the marginal multiplier is correct.
In summary the attack surface is now greatly reduced, also the maximum number of stakes for a user is 25 which limits how fragmented deposits can be.
Description:
Note: This issue has been found during the mitigation review.
The MuteAmplifierRedux contract downstream relies on the MuteOracle.
The calculation of LP token value relies on reserve0, reserve1 and totalSupply of the underlying pool (Link).
Also, token prices are queried with the current() function (Link). This is a TWAP price but the sample period can be as small as 1 second (Link).
The reserve values can be manipulated via a flash-loan attack. The cost to the attacker is simply the pool fee plus the flash loan fee. The manipulation of the reserve values allows the attacker to increase the valuation of his LP tokens, thereby earning far greater rewards from MuteAmplifierRedux.
The totalSupply cannot be manipulated with a for-profit motive since an increased totalSupply always leads to lower LP token valuation. This could simply be used in a front-running attack to grief another user, decreasing the valuation of his LP token. This would not be possible in a flash-loan attack though since the attack must be sustained for at least one block.
Similarly the current() price cannot be manipulated in a flash-loan attack, since the attack must be sustained for at least one block. Still, the sample period can be as small as one second and so it is likely to be a profitable attack in some cases.
Impact:
The MuteOracle can be manipulated at low cost, relative to the reward that an attacker gets from MuteAmplifierRedux.
Mitigation Review:
reserve0 and reserve1 are now calculated based on a 30 minute time weighted average (Link).
The pool prices are TWAP prices based on the same 30 minutes timeframe as the reserves (Link).
This means that reserve0, reserve1 and the pool prices are sufficiently robust against manipulation attempts such that it should not be possible to manipulate them with a net positive payoff from MuteAmplifierRedux.
Still, the totalSupply is the spot value and can be manipulated. The griefing attack described above is still possible.
Description:
The purpose of the withdrawEmergency() function is to be used when there are fee / reward calculation errors (Link).
However, the function calls IMuteSwitchPairDynamic(lpToken).claimFees() (Link).
Impact:
The purpose of the withdrawEmergency() function is circumvented. If there is an issue with the fee calculation, users may not be able to execute a emergency withdrawal.
Recommendation:
It is not necessary to call the claimFees() function. So the call should be removed to make emergency withdrawals truly independent from fee calculations.
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..e0f6a3e 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -377,8 +377,6 @@ contract MuteAmplifierRedux is ReentrancyGuard {
uint256 lpTot;
uint256 payoutDecrease;
- IMuteSwitchPairDynamic(lpToken).claimFees();
-
for(uint256 i = _stakes.length - 1; i >= 0; i--){
lpTot += _stakes[i].stakeAmount;Mitigation Review:
The call to IMuteSwitchPairDynamic(lpToken).claimFees() has been removed as recommended.
In addition, the call to customTreasury.decreasePendingPayout() is now wrapped in a try-catch block (Link).
This addresses a centralization concern in which a malicious owner of the customTreasury could make this line revert by manually decreasing the pending payout such as to cause an underflow when withdrawEmergency() calls customTreasury.decreasePendingPayout().
Description:
The _deposit() function sends part of the deposited lpTokens as a management_fee to the customTreasury contract (Link).
The lpTokens constantly earn fees from swaps. However, the customTreasury contract has no function to collect the fees (Link).
Impact:
The issue is a direct loss of funds for the management.
The size of the loss depends on the amount of lpTokens in the contract and the amount of fees they accrue before they are withdrawn.
Recommendation:
Implement a function in the MuteAmplifierTreasury contract that allows to claim the fees that the lpTokens accrue by calling IMuteSwitchPairDynamic(lpToken).claimFees().
iff --git a/contracts/amplifierv2/MuteAmplifierTreasury.sol b/contracts/amplifierv2/MuteAmplifierTreasury.sol
index 8590eca..e0560d4 100644
--- a/contracts/amplifierv2/MuteAmplifierTreasury.sol
+++ b/contracts/amplifierv2/MuteAmplifierTreasury.sol
@@ -4,6 +4,8 @@ import "@openzeppelin/contracts/access/Ownable.sol";
import '../libraries/TransferHelper.sol';
import '../interfaces/IERC20.sol';
+import '../interfaces/IMuteSwitchPairDynamic.sol';
+
contract MuteAmplifierTreasury is Ownable {
/* ======== STATE VARIABLS ======== */
@@ -77,4 +79,8 @@ contract MuteAmplifierTreasury is Ownable {
ampContract[_ampContract] = false;
emit AmpContractRemoved(_ampContract);
}
+
+ function claimFees(address _lpToken) external onlyOwner {
+ IMuteSwitchPairDynamic(_lpToken).claimFees();
+ }
}Mitigation Review:
This issue has been fixed as recommended.
Description:
The calculateMultiplier() function downstream depends on veMute.getPastVotes(), which is a function inherited from OZ's Votes.sol (Link).
This issue is the same that was reported in a previous C4 audit: code-423n4/2023-03-mute-findings#36
Impact:
Multiple users can earn rewards based on the same voting power. Users can delegate their voting power to each other to increase their rewards.
Recommendation:
A relatively simple solution could be to not query the getPastVotes() from the current time, but to query it every epochDuration time.
Say epochDuration = 100. All deposits between timestamp 0 and timestamp 99 would then use getPastVotes(0), deposits between timestamp 100 and timestamp 199 use getPastVotes(100) and so on.
This ensures that each vote can only be used for multiplying rewards for one user at once.
However, user still only need to hold their votes for a single block to be eligible for increased rewards for the whole epochDuration.
Mitigation Review:
A better fix has been made than what was suggested in the "Recommendation".
The veTokens that hold the voting power are tracked upon making deposits (Link).
It is ensured that a given unit of voting power can only be used for one stake at any given time.
Once the epochDuration for the stake has passed, the veToken can be used in another stake.
Description:
Fee tokens, i.e., tokens of the pool, can be any tokens including tokens that don't have a return value like USDT.
Therefore, the TransferHelper.safeTransfer() function should be used to transfer them.
Impact:
Fee token transfers may fail because some tokens like USDT don't return a bool.
Recommendation:
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..950d78a 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -6,6 +6,8 @@ import '../interfaces/IERC20.sol';
import '../interfaces/IMuteSwitchPairDynamic.sol';
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+import '../libraries/TransferHelper.sol';
+
contract MuteAmplifierRedux is ReentrancyGuard {
/* ======== EVENTS ======== */
@@ -268,11 +270,11 @@ contract MuteAmplifierRedux is ReentrancyGuard {
}
if(feeToken0 > 0){
- IERC20(IMuteSwitchPairDynamic(lpToken).token0()).transfer(msg.sender, feeToken0);
+ TransferHelper.safeTransfer(IMuteSwitchPairDynamic(lpToken).token0(), msg.sender, feeToken0);
}
if(feeToken1 > 0){
- IERC20(IMuteSwitchPairDynamic(lpToken).token1()).transfer(msg.sender, feeToken1);
+ TransferHelper.safeTransfer(IMuteSwitchPairDynamic(lpToken).token1(), msg.sender, feeToken1);
}
}
@@ -360,11 +362,11 @@ contract MuteAmplifierRedux is ReentrancyGuard {
}
if(feeToken0 > 0){
- IERC20(IMuteSwitchPairDynamic(lpToken).token0()).transfer(msg.sender, feeToken0);
+ TransferHelper.safeTransfer(IMuteSwitchPairDynamic(lpToken).token0(), msg.sender, feeToken0);
}
if(feeToken1 > 0){
- IERC20(IMuteSwitchPairDynamic(lpToken).token1()).transfer(msg.sender, feeToken1);
+ TransferHelper.safeTransfer(IMuteSwitchPairDynamic(lpToken).token1(), msg.sender, feeToken1);
}
}Mitigation Review:
This issue has been fixed as recommended.
The TransferHelper.safeTransfer() library function is used.
Description:
The pool fee calculation for the users in the MuteAmplifierRedux contract (Link) mirrors the calculation that is made for MuteAmplifierRedux in the MuteSwitchPairDynamic contract (Link).
It is in theory possible that the rounding of the MuteAmplifierRedux's rewards is less favorable than the rounding of the users' fees in the MuteAmplifierRedux contract.
This means that if the last user tries to redeem his pool fees, this would cause a revert since the MuteAmplifierRedux's balance is insufficient.
Impact:
The user would need to send a small amount of the pool fee tokens to the MuteAmplifierRedux contract for the transfer to succeed.
So the impact is a temporary DoS.
Recommendation:
Limit the fee amount to be transferred to the balance of the contract.
In case there exists the described rounding issue, the redeemption / withdrawal can still succeed.
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..e38574f 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -268,11 +268,11 @@ contract MuteAmplifierRedux is ReentrancyGuard {
}
if(feeToken0 > 0){
+ IERC20 token = IERC20(IMuteSwitchPairDynamic(lpToken).token0());
- IERC20(IMuteSwitchPairDynamic(lpToken).token0()).transfer(msg.sender, feeToken0);
+ token.transfer(msg.sender, clamp_value(feeToken0,token.balanceOf(address(this))));
}
if(feeToken1 > 0){
+ IERC20 token = IERC20(IMuteSwitchPairDynamic(lpToken).token1());
- IERC20(IMuteSwitchPairDynamic(lpToken).token1()).transfer(msg.sender, feeToken1);
+ token.transfer(msg.sender, clamp_value(feeToken1,IERC20(token.balanceOf(address(this))));
}
}
@@ -360,11 +360,11 @@ contract MuteAmplifierRedux is ReentrancyGuard {
}
if(feeToken0 > 0){
+ IERC20 token = IERC20(IMuteSwitchPairDynamic(lpToken).token0());
- IERC20(IMuteSwitchPairDynamic(lpToken).token0()).transfer(msg.sender, feeToken0);
+ token.transfer(msg.sender, clamp_value(feeToken0,token.balanceOf(address(this))));
}
if(feeToken1 > 0){
+ IERC20 token = IERC20(IMuteSwitchPairDynamic(lpToken).token1());
- IERC20(IMuteSwitchPairDynamic(lpToken).token1()).transfer(msg.sender, feeToken1);
+ token.transfer(msg.sender, clamp_value(feeToken1,token.balanceOf(address(this))));
}
}Mitigation Review:
This issue has been fixed as recommended.
The amount of fee tokens to send is limited to the MuteAmplifierRedux contracts's balance.
Description:
The stakes of a given user are saved in a StakeTerms[] array.
All functions that allow a user to get back his deposited lpTokens require to iterate over the whole array.
The size of the array can be so big that the transaction cannot execute, and since the array cannot get shorter without iterating over it, the lpTokens are stuck.
Impact:
Only a user himself can stake on his behalf, so for a single user this is not a realistic scenario.
However, it is possible that a third party protocol integrates with the MuteAmplifierRedux contract that serves multiple users.
Thereby any of the users would potentially be able to increase the StakeTerms[] array on behalf of the third party contract, being able to cause a DoS.
Recommendation:
The most straightforward solution is to limit the size of the StakeTerms[] array to a reasonable value, e.g., 100.
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..cfd174d 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -171,6 +171,8 @@ contract MuteAmplifierRedux is ReentrancyGuard {
require(payout >= 10**18, "Stake too small" ); // must be > 1 payout token ( underflow protection )
// make sure we are not requiring more rewards than there are available
require(payout <= maxDeposit(), "Stake too large"); // size protection incase there is not enough rewards
+
+ require(userStakes[_depositor].length < 100);
// pay management fee after rewards are calculated
if(management_fee != 0){Mitigation Review:
The _deposit() function checks that the number of stakes for a user cannot exceed 25 (Link).
The issue is fixed.
The IVeMute interface is never used, so it can be removed.
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..1352888 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -458,10 +458,4 @@ interface IMuteAmplifierTreasury {
function increasePendingPayout(uint _amountPayoutToken) external;
function decreasePendingPayout(uint _amountPayoutToken) external;
function totalPendingPayouts() external view returns (uint);
-}
-
-interface IVeMute {
- function balanceOf(address account) external view returns(uint256 amount);
- function getPriorVotes(address account, uint256 block) external view returns(uint256 amount);
- function LockTo(uint256 _amount, uint256 _lock_time, address to) external;
}Mitigation Review:
The recommended improvement has been implemented.
The MuteAmplifierTreasury contract does not have a valueOfToken() function so it should be removed from the IMuteAmplifierTreasury interface.
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..a508c75 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -452,7 +452,6 @@ contract MuteAmplifierRedux is ReentrancyGuard {
interface IMuteAmplifierTreasury {
function sendPayoutTokens(uint _amountPayoutToken, address receiver) external;
- function valueOfToken( address _principalTokenAddress, uint _amount ) external view returns ( uint value_ );
function payoutToken() external view returns (address);
function owner() external view returns (address);
function increasePendingPayout(uint _amountPayoutToken) external;Mitigation Review:
The recommended improvement has been implemented.
Some of the events use parameter names that do not reflect the actual values that are emitted.
The following names are more descriptive:
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..13cb5e2 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -10,8 +10,8 @@ contract MuteAmplifierRedux is ReentrancyGuard {
/* ======== EVENTS ======== */
event StakeCreated(uint deposit, uint payout, address depositor, uint time);
- event EpochDurationChanged(uint _payout);
- event RewardBaseChanged(uint _lock);
+ event EpochDurationChanged(uint _duration);
+ event RewardBaseChanged(uint _rewardBase);
event ManagementFeeChanged(uint _fee);
event PercentageMultiChanged(uint _pMulti);Mitigation Review:
The recommended improvement has been implemented.
treasury, ampRatio and veMuteToken are never used / no longer needed.
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..5395b14 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -18,7 +18,6 @@ contract MuteAmplifierRedux is ReentrancyGuard {
/* ======== STATE VARIABLES ======== */
address immutable private muteToken; // token paid for principal
- address immutable private veMuteToken;
address immutable private lpToken; // inflow token
IMuteAmplifierTreasury immutable private customTreasury; // pays for and receives principal
IMuteOracle oracle;
@@ -46,8 +45,6 @@ contract MuteAmplifierRedux is ReentrancyGuard {
uint256 public percentageMulti; // minimum percentage set in place for modification of reward boost [1e18,2e18]
uint256 public management_fee; // lp withdrawal fee bps
- address public treasury; // address that receives the lp withdrawal fee
- uint256 public ampRatio;
/* ======== CONSTRUCTOR ======== */
@@ -55,18 +52,16 @@ contract MuteAmplifierRedux is ReentrancyGuard {
* @notice initializes the staking contract
* @param _customTreasury address
* @param _lpToken address
- * @param _veMuteToken address
* @param _rewardBase uint
* @param _oracle address
* @param _mgmt_fee uint
* @param _percentageMulti uint
*/
- constructor(address _customTreasury, address _lpToken, address _veMuteToken, uint256 _rewardBase,
+ constructor(address _customTreasury, address _lpToken, uint256 _rewardBase,
address _oracle, uint256 _mgmt_fee, uint256 _percentageMulti) {
//safety checks made in factory
customTreasury = IMuteAmplifierTreasury( _customTreasury );
muteToken = IMuteAmplifierTreasury(_customTreasury).payoutToken();
- veMuteToken = _veMuteToken;
lpToken = _lpToken; // must be paired with ETH / USDC / USDT
management_fee = _mgmt_fee; //bps 10k
@@ -75,9 +70,6 @@ contract MuteAmplifierRedux is ReentrancyGuard {
// mute usd rewards to lp val - apy, bps
rewardBase = _rewardBase; // 0 = 0%, 1000 = 10%
oracle = IMuteOracle(_oracle);
-
- // approve lock token to spend payout
- IERC20(muteToken).approve(veMuteToken, type(uint256).max);
}
/**Mitigation Review:
The recommended improvement has been implemented.
Checking block.timestamp >= expiry instead of block.timestamp > expiry triggers the more efficient calculation and avoids that a stake that has already been fully paid out must be iterated upon again, leading to slightly lower Gas usage.
diff --git a/contracts/amplifierv2/MuteAmplifierRedux.sol b/contracts/amplifierv2/MuteAmplifierRedux.sol
index 074630e..e3da717 100644
--- a/contracts/amplifierv2/MuteAmplifierRedux.sol
+++ b/contracts/amplifierv2/MuteAmplifierRedux.sol
@@ -238,7 +238,7 @@ contract MuteAmplifierRedux is ReentrancyGuard {
}
- if(block.timestamp > expiry){
+ if(block.timestamp >= expiry){
lpTot = lpTot + _stakes[i].stakeAmount;
rewardTot = rewardTot + remainingRewards;
@@ -288,7 +288,7 @@ contract MuteAmplifierRedux is ReentrancyGuard {
uint256 expiry = _stakes[i].stakedTime + epochDuration;
uint256 remainingRewards = _stakes[i].rewardAmount - _stakes[i].rewardClaimed;
- if(block.timestamp > expiry){
+ if(block.timestamp >= expiry){
rewardTot = rewardTot + remainingRewards;
} else {
@@ -334,7 +334,7 @@ contract MuteAmplifierRedux is ReentrancyGuard {
lpTot = lpTot + _stakes[i].stakeAmount;
- if(block.timestamp > expiry){
+ if(block.timestamp >= expiry){
rewardTot = rewardTot + remainingRewards;
} else {
rewardTot = rewardTot + (remainingRewards * (block.timestamp - _stakes[i].stakedTime) / epochDuration);Mitigation Review:
The recommended improvement has been implemented.