Mute Switch - Versus contest - HollaDieWaldfee's results

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L153-L200

Vulnerability details

Impact

The MuteBond.deposit function allows the user to purchase a bond with LP tokens and receive MUTE tokens in return.

The bondPrice increases linearly over time (which I should mention means the bond gets cheaper; the naming is a bit confusing). There is another mechanic that changes the bondPrice: Whenever a bond is purchased the bondPrice decreases (making the bond more expensive; meaning the user gets less MUTE for the LP tokens he provides).

The user may also provide a max_buy=true parameter which means he will purchase the remaining MUTE such that a new epoch is entered.

There are several scenarios possible that lead to unexpected unintended outcomes for the user (Essentially a loss of funds). I decided to put all of this into a single report since all scenarios come down to this:

1. When the user calls the deposit function he cannot specify how many MUTE tokens he wants to get out at a minimum. The user cannot know if and how much the bondPrice changes in between the time he creates the transaction and the time the transaction is processed. The bondPrice can also be changed by the owner by setting startPrice, maxPrice or epochDuration within an ongoing epoch.

2. Also if he sets max_buy=true and a new epoch is entered by the time the transaction is processed or the owner has increased maxPayout, the user ends up paying a lot more of his LP tokens than intended. Sure he may only approve a certain number. But many users will just approve the maximum amount.

I think the first set of scenarios where the bondPrice changes is more severe because even a user that does not make the "mistake" of approving type(uint256).max is prone to it.

So I focus on this first set of scenarios in my proof of concept. I provide a solution for both of these problems in the recommendations section.

Proof of Concept

Add the following test to the bonds.ts test file.

it('POC unexpected price', async function () {
    // halfway into the duration
    var set_time = (await time.latest()).plus(60 * 60 * 24 * 3.5)
    await time.increaseTo(set_time)

    // Buyer1 intends to call "deposit" at the current price
    console.log("Intended bond price");
    console.log(await bondContract.bondPrice());

    // Other transactions are executed before his
    // They decrease "bondPrice"
    await bondContract.connect(owner).deposit(new BigNumber(1).times(Math.pow(10,18)).toFixed(), owner.address, false)
    
    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(owner).deposit(new BigNumber(1).times(Math.pow(10,18)).toFixed(), owner.address, false)
    
    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(owner).deposit(new BigNumber(1).times(Math.pow(10,18)).toFixed(), owner.address, false)
    // This means the user receives less MUTE than expected.
    // He may not want his transaction to be executed at such a bad price
    console.log("Actual bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(10).times(Math.pow(10,18)).toFixed(), buyer1.address, false)
  })

Due to the other transactions executing before the user's, the bondPrice drops which means the user gets less MUTE than expected. Essentially purchasing MUTE at a worse price than expected which is a loss of funds. The user might be better off by just keeping his LP tokens.

Tools Used

VSCode

Recommended Mitigation Steps

How can the first set of scenarios be mitigated?
The user should be able to provide a minPayout parameter that specifies how many MUTE tokens he wants to receive at a minimum. Thereby if the bondPrice changes and he would receive less MUTE than intended, he is protected.

For the second set of scenarios (where max_buy=true) I propose that the already existing value parameter should specify a maxValue, i.e. how many LP tokens the user is willing to pay at a maximum.

Both fixes together then look like this:

diff --git a/contracts/bonds/MuteBond.sol b/contracts/bonds/MuteBond.sol
index 96ee755..407e9e6 100644
--- a/contracts/bonds/MuteBond.sol
+++ b/contracts/bonds/MuteBond.sol
@@ -150,11 +150,13 @@ contract MuteBond {
      *  @param _depositor address
      *  @param max_buy bool
      */
-    function deposit(uint value, address _depositor, bool max_buy) external returns (uint) {
+    function deposit(uint value, address _depositor, bool max_buy, uint minPayout) external returns (uint) {
         // amount of mute tokens
         uint payout = payoutFor( value );
         if(max_buy == true){
+          uint maxValue = value;
           value = maxPurchaseAmount();
+          require(value <= maxValue, "Bond too large");
           payout = maxDeposit();
         } else {
           // safety checks for custom purchase
@@ -163,7 +165,7 @@ contract MuteBond {
           require( payout <= maxDeposit(), "Deposit too large"); // size protection because there is no slippage
         }
 
-
+        require(payout >= minPayout, "Payout too small");
         // total debt is increased
         totalDebt = totalDebt.add( value );
         totalPayoutGiven = totalPayoutGiven.add(payout); // total payout increased

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L185-L187 https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L161

Vulnerability details

Impact

The bondPrice in the MuteBond contract increases linearly during the epochDuration from startPrice in the beginning to maxPrice in the end.

The bondPrice determines how many MUTE tokens a user receives for bonding his LP tokens. The higher the bondPrice the more MUTE tokens a user receives for bonding LP tokens (Link). This is to incentivise bonding.

In order to understand this issue it is important to understand a second mechanism that impacts the bondPrice: Every call to MuteBond.deposit increases epochStart by 5% of timeElapsed:

Link

// adjust price by a ~5% premium of delta
uint timeElapsed = block.timestamp - epochStart;
epochStart = epochStart.add(timeElapsed.mul(5).div(100));

The result of this is that the price discount, i.e. how much the bondPrice is above startPrice, is reduced by 5%. The reason I call it discount even though it's an increase is that it discounts the price of MUTE relative to LP tokens.

I discussed this with the sponsor and the reason they implemented this mechanism is to reflect supply and demand. The more bonds people create (higher demand) the higher the price they need to pay (discount is lowered which means they get less MUTE).
Similarly if the demand is low the discount is not lowered and people are incentivised to create bonds. The calculation does not need to be exact it's just a rough mechanism.

Now, the issue is that the minimum amount of LP tokens to call the deposit function with is 0.01 * 1e18, i.e. one hundreth of a token. (Assuming that bondPrice is 1e18 since you need to multiply by bondPrice to get from LP token amount to MUTE token amount)

At the current value of the LP token of the MUTE / ETH pool this is 0.01 * $88 = $0.88.

As of now, 1 LP token can withdraw 39.78 MUTE and 0.02513 ETH which is ~$88.

This means that with a deposit of only $0.88 an attacker can influence this mechanism that should reflect supply and demand.

By depositing such a small amount of LP tokens we cannot speak of actual supply and demand determining the price. The attacker is just manipulating the discount, preventing it to go up.

The sponsor agrees with this and suggested to increase the minimum deposit such that a deposit reflects an actual demand.

Proof of Concept

Add the following test to the dao.ts test file.

it('Purchase exact amount of bond', async function () {
    // halfway into the duration
    var set_time = (await time.latest()).plus(60 * 60 * 24 * 3.5)
    await time.setNextBlockTime(set_time)

    // legitimate transaction to buy a bond
    await bondContract.connect(buyer1).deposit(new BigNumber(10).times(Math.pow(10,18)).toFixed(), buyer1.address, false)

    // now the attacker makes the price drop; meaning the bond becomes more expensive
    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)

    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)
    
    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)

    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)

    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)
    
    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)

    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)

    console.log("Bond price");
    console.log(await bondContract.bondPrice());
    await bondContract.connect(buyer1).deposit(new BigNumber(1).times(Math.pow(10,16)).toFixed(), buyer1.address, false)
  })

The attacker makes the price drop from 145125992063492063492 to 133172949735449735449 without any real deposit volume occurring.

The attacker deposited 0.01 LP tokens 8 times. As explained above the price should only drop (making the bond more expensive) when there is actual demand.

Tools Used

VSCode

Recommended Mitigation Steps

I discussed this issue with the sponsor and they suggested that the minimum payout amount should be 10% of maxPayout. I agree that a payout of >=10%*maxPayout reflects actual demand and therefore is a reasonable value.

Therefore the suggested fix looks like this:

diff --git a/contracts/bonds/MuteBond.sol b/contracts/bonds/MuteBond.sol
index 96ee755..d16a1b0 100644
--- a/contracts/bonds/MuteBond.sol
+++ b/contracts/bonds/MuteBond.sol
@@ -159,6 +159,7 @@ contract MuteBond {
         } else {
           // safety checks for custom purchase
           require( payout >= ((10**18) / 100), "Bond too small" ); // must be > 0.01 payout token ( underflow protection )
+          require( payout >= (maxPayout / 10), "Bond too small" ); 
           require( payout <= maxPayout, "Bond too large"); // size protection because there is no slippage
           require( payout <= maxDeposit(), "Deposit too large"); // size protection because there is no slippage
         }

The sponsor may also explore ways to adjust the price by an amount that is weighted by the payout amount. I.e. lower payout -> less price adjustment, higher payout -> more price adjustment

Judge has assessed an item in Issue #13 as 2 risk. The relevant finding follows:

Lines of code https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L153-L200

Vulnerability details Impact The MuteBond.deposit function allows the user to purchase a bond with LP tokens and receive MUTE tokens in return.

The bondPrice increases linearly over time (which I should mention means the bond gets cheaper; the naming is a bit confusing). There is another mechanic that changes the bondPrice: Whenever a bond is purchased the bondPrice decreases (making the bond more expensive; meaning the user gets less MUTE for the LP tokens he provides).

The user may also provide a max_buy=true parameter which means he will purchase the remaining MUTE such that a new epoch is entered.

There are several scenarios possible that lead to unexpected unintended outcomes for the user (Essentially a loss of funds). I decided to put all of this into a single report since all scenarios come down to this:

When the user calls the deposit function he cannot specify how many MUTE tokens he wants to get out at a minimum. The user cannot know if and how much the bondPrice changes in between the time he creates the transaction and the time the transaction is processed. The bondPrice can also be changed by the owner by setting startPrice, maxPrice or epochDuration within an ongoing epoch.

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/dao/dMute.sol#L90-L129 https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/dao/dMute.sol#L135-L139

Vulnerability details

Impact

This report deals with how an attacker can abuse the fact that he can lock MUTE tokens for any other user and thereby push items to the array of UserLockInfo structs of the user.

There are two functions in the dMute contract that iterate over all items in this array (RedeemTo and GetUnderlyingTokens).

Thereby if the attacker pushes sufficient items to the array of a user, he can make the above two functions revert since they require more Gas than the Block Gas Limit.

According to the zkSync documentation the block gas limit is currently 12.5 million (Link).

The attack is of "High" impact for the RedeemTo function since this function needs to succeed in order for the user to redeem his MUTE tokens.

The user might have a lot of MUTE tokens locked and the attacker can make it such that they can never be redeemed. The attacker cannot gain a profit from this attack, i.e. he cannot steal anything, but due to the possibility of this attack users will not lock their tokens, especially not a lot of them.

This is all the more severe because the MuteBond and MuteAmplifier contracts also rely on the locking functionality so those upstream features can also not be used securely.

In the Mitigation section I will show how the GetUnderlyingTokens function can be made to run in $O(1)$ time instead of $O(lock:array:length)$.

The RedeemTo function can be made to run in $O(indexes:array:length)$ instead of $O(lock:array:length)$. The length of the indexes array is determined by the user and simply tells how many locked items to redeem. So there is no possibility of DOS.

Proof of Concept

Note: a redemption costs ~7 million Gas when 1000 items are locked. So when running on the zkSync network even 2000 items should be enough. The hardhat tests use a local Ethereum network instead of a fork of zkSync so in order to hit 30 million Gas (which is the Ethereum block gas limit) we need to add more items to the queue.

You can add the following test to the dao.ts test file:

it('Lock DOS', async function () {
    var tx = await muteToken.approve(dMuteToken.address, MaxUint256)


    let lock_time_week = new BigNumber(60 * 60 * 24 * 7);
    let max_lock = lock_time_week.times(52);

    let lock_amount = new BigNumber(1).times(Math.pow(10,2))

    // @audit fill up array
    for(let i=0;i<5000;i++) {
        tx = await dMuteToken.LockTo(lock_amount.toFixed(0), lock_time_week.toFixed(),owner.address)
    }

    await time.increase(60 * 60 * 24 * 7)

    tx = await dMuteToken.Redeem([0])
})

It adds 5000 lock items to the array of the owner address. When the owner then tries to redeem even a single lock the transaction fails due to an out of gas error.

(Sometimes it reverts with TransactionExecutionError: Transaction ran out of gas error sometimes it reverts due to timeout. If you try a few times it should revert with the out of gas error.)

The amount of MUTE tokens that the attacker loses to execute this attack is negligible. As you can see in the test 100 Wei * 5000 = 500,000 Wei is sufficient (There needs to be some amount of MUTE such that the LockTo function does not revert). The only real cost comes down to Gas costs which are cheap on zkSync.

Tools Used

VSCode

Recommended Mitigation Steps

First for the GetUnderlyingTokens function: The contract should keep track of underlying token amounts for each user in a mapping that is updated with every lock / redeem call. The GetUnderlyingTokens function then simply needs to return the value from this mapping.

Secondly, fixing the issue with the RedeemTo function is a bit harder. I discussed this with the sponsor and I have been told they don't want this function to require an already sorted lock_index array as parameter. So the lock_index array can contain indexes in random order.

This means it must be sorted internally. Depending on the expected length of the lock_index array different sorting algorithms may be used. I recommend to use an algorithm like quick sort to allow for many indexes to be specified at once.

I will use a placeholder for the sorting algorithm for now so the sponsor may decide which one to use.

The proposed fixes for both functions are then like this:

diff --git a/contracts/dao/dMute.sol b/contracts/dao/dMute.sol
index 59f95b7..11d21fb 100644
--- a/contracts/dao/dMute.sol
+++ b/contracts/dao/dMute.sol
@@ -18,6 +18,7 @@ contract dMute is dSoulBound {
     }
 
     mapping(address => UserLockInfo[]) public _userLocks;
+    mapping(address => uint256) public _amounts;
 
     uint private unlocked = 1;
 
@@ -79,6 +80,7 @@ contract dMute is dSoulBound {
         _mint(to, tokens_to_mint);
 
         _userLocks[to].push(UserLockInfo(_amount, block.timestamp.add(_lock_time), tokens_to_mint));
+        _amounts[to] = _amounts[to] + _amount;
 
         emit LockEvent(to, _amount, tokens_to_mint, _lock_time);
     }
@@ -91,8 +93,14 @@ contract dMute is dSoulBound {
         uint256 total_to_redeem = 0;
         uint256 total_to_burn = 0;
 
-        for(uint256 i; i < lock_index.length; i++){
-          uint256 index = lock_index[i];
+        ///////////////////////////////////////////////
+        //                                           //
+        // sort lock_index array in ascending order //
+        //                                          //
+        //////////////////////////////////////////////
+
+        for(uint256 i = lock_index.length; i > 0; i--){
+          uint256 index = lock_index[i - 1];
           UserLockInfo memory lock_info = _userLocks[msg.sender][index];
 
           require(block.timestamp >= lock_info.time, "dMute::Redeem: INSUFFICIENT_LOCK_TIME");
@@ -102,23 +110,14 @@ contract dMute is dSoulBound {
           total_to_redeem = total_to_redeem.add(lock_info.amount);
           total_to_burn = total_to_burn.add(lock_info.tokens_minted);
 
-          _userLocks[msg.sender][index] = UserLockInfo(0,0,0);
+          _userLocks[msg.sender][index] = _userLocks[msg.sender][_userLocks[msg.sender].length - 1];
+          _userLocks[msg.sender].pop();
         }
 
         require(total_to_redeem > 0, "dMute::Lock: INSUFFICIENT_REDEEM_AMOUNT");
         require(total_to_burn > 0, "dMute::Lock: INSUFFICIENT_BURN_AMOUNT");
 
-
-        for(uint256 i = _userLocks[msg.sender].length; i > 0; i--){
-          UserLockInfo memory lock_info = _userLocks[msg.sender][i - 1];
-
-          // recently redeemed lock, destroy it
-          if(lock_info.time == 0){
-            _userLocks[msg.sender][i - 1] = _userLocks[msg.sender][_userLocks[msg.sender].length - 1];
-            _userLocks[msg.sender].pop();
-          }
-        }
-
+        _amounts[msg.sender] = _amounts[msg.sender] + total_to_redeem;
         //redeem tokens to user
         IERC20(MuteToken).transfer(to, total_to_redeem);
         //burn dMute
@@ -133,8 +132,6 @@ contract dMute is dSoulBound {
     }
 
     function GetUnderlyingTokens(address account) public view returns(uint256 amount) {
-        for(uint256 i; i < _userLocks[account].length; i++){
-          amount = amount.add(_userLocks[account][i].amount);
-        }
+        return _amounts[account];
     }
 }

Judge has assessed an item in Issue #17 as 2 risk. The relevant finding follows:

[L-07] First user that stakes again after a period without stakers receives too many rewards The MuteAmplifier contract pays out rewards on a per second basis. Let's assume there is only 1 staker which is Bob.

Say Bob calls stake at timestamp 0 and calls withdraw at timestamp 10. He receives rewards for 10 seconds of staking.

At timestsamp 30 Bob calls stake again (there were no stakers from timestamp 10 to timestamp 30). If Bob calls withdraw at say timestamp 40, he receives not only rewards for the 10 seconds he has staked but for 30 seconds (timestamp 10 to timestamp 40).

This means that whenever there are temporarily no stakers, whoever first stakes again receives all the rewards from the previous period without stakers.

This is due to how the update modifier works.

When someone stakes and there were no other stakers, the if block is not entered and the _mostRecentValueCalcTime variable is not updated.

So when the update modifier is executed again the staker also receives the rewards from the period when there were no stakers.

I just want to make the sponsor aware of this behavior. The sponsor may decide that this is unintended and needs to change. I think this might even be a beneficial behavior because it incentivises users to stake if there are no stakers because they will get more rewards.

Judge has assessed an item in Issue #17 as 2 risk. The relevant finding follows:

[L-05] Check that staking cannot occur when endTime is reached The MuteAmplifier.stake function should require that the current timestamp is smaller than endTime even when the call to stake is the first that ever occurred. Currently the check is only made in the case that the call to stake is not the first. The check should be made in both cases. This is because when staking occurs when block.timestamp >= endTime, no rewards will be paid out. Additionally the user needs to pay the management fee on his LP token stake. So there is really no point in allowing users to do it because it only hurts them.

Fix:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol index 9c6fcb5..460c408 100644 --- a/contracts/amplifier/MuteAmplifier.sol +++ b/contracts/amplifier/MuteAmplifier.sol @@ -202,13 +202,12 @@ contract MuteAmplifier is Ownable{ */ function stake(uint256 lpTokenIn) external virtual update nonReentrant { require(lpTokenIn > 0, "MuteAmplifier::stake: missing stake");

•    require(block.timestamp < endTime, "MuteAmplifier::stake: staking is over");
     require(block.timestamp >= startTime && startTime !=0, "MuteAmplifier::stake: not live yet");
     require(IERC20(muteToken).balanceOf(address(this)) > 0, "MuteAmplifier::stake: no reward balance");
  
     if (firstStakeTime == 0) {
         firstStakeTime = block.timestamp;
  

•    } else {
  

•        require(block.timestamp < endTime, "MuteAmplifier::stake: staking is over");
     }
  
     lpToken.safeTransferFrom(msg.sender, address(this), lpTokenIn);
  

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L153-L200

Vulnerability details

Impact

I will show in this report how the MuteBond.deposit function can experience a temporary DOS.

The attacker or just any other user by mistake or by not knowing about it can receive a payout from the deposit function that puts the payoutTotal of the current epoch just below the maxPayout (up to 52 Wei below maxPayout as we will see).

This means that the next calls to deposit can at a max receive a payout of this small amount. However if the dMute.LockTo function is called with such a small amount it reverts in this line since it rounds the dMute tokens to mint to 0 which is not allowed.

This means that the MuteBond.deposit function cannot be called anymore and a new epoch cannot be entered.

This would be of High severity if it wasn't possible for the owner to increase maxPayout and resolve the DOS.

However the current behavior is certainly not intended and takes some time to resolve so there is definitely an impact to the availability of the MuteBond.deposit function which should be available at all times. So I determine that this issue is of Medium severity.

Proof of Concept

I tried in Remix with the following function how big the _amount parameter in the LockTo function can be such that the result which is tokens_to_mint is rounded to 0:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.18;
contract Test  {
    uint256 depositFee = 1000;

    function timeToTokens(uint256 _amount) public pure returns (uint256){
        uint256 week_time = 1 weeks;
        uint256 max_lock = 52 weeks;
        uint256 _lock_time = 1 weeks;

        require(_lock_time >= week_time, "dMute::Lock: INSUFFICIENT_TIME_PARAM");
        require(_lock_time <= max_lock, "dMute::Lock: INSUFFICIENT_TIME_PARAM");

        // amount * % of time locked up from min to max
        uint256 base_tokens = (_amount * (_lock_time * 10**18 / max_lock)) / (10**18);
        // apply % min max bonus
        //uint256 boosted_tokens = base_tokens.mul(lockBonus(lock_time)).div(10**18);

        return base_tokens;
    }
}

_amount can be as big as 52 Wei. Beginning from 53 Wei it won't be rounded to 0.

So let's assume to make the calculations simpler that the following conditions apply:

maxPayout = 1e18
startPrice = 1e18
block.timestamp == epochStart (start price is the current price)

When we now call MuteBond.deposit with value=1e18 - 1 the payout is also 1e18 - 1.

This means the remaining payout to be made in this epoch is 1 Wei.

This will cause a revert in the dMute.LockTo function.

Tools Used

VSCode

Recommended Mitigation Steps

I recommend that if after a call to MuteBond.deposit the difference between terms[epoch].payoutTotal and maxPayout is very small (e.g. 0.01 * 10**18) the new epoch should be entered even without reaching the exact maxPayout amount. This ensures that the dMute.LockTo function can never revert.

Fix:

diff --git a/contracts/bonds/MuteBond.sol b/contracts/bonds/MuteBond.sol
index 96ee755..d900848 100644
--- a/contracts/bonds/MuteBond.sol
+++ b/contracts/bonds/MuteBond.sol
@@ -190,7 +190,7 @@ contract MuteBond {
           epochStart = block.timestamp;
 
         // exhausted this bond, issue new one
-        if(terms[epoch].payoutTotal == maxPayout){
+        if(maxPayout - terms[epoch].payoutTotal < (1e16)){
             terms.push(BondTerms(0,0,0));
             epochStart = block.timestamp;
             epoch++;

Judge has assessed an item in Issue #17 as 2 risk. The relevant finding follows:

[L-10] It is possible in theory that stakes get locked due to call to LockTo with very small reward amount I pointed out and explained in my report #7 MuteBond.sol: deposit function reverts if remaining payout is very small due to >0 check in dMute.LockTo function how the MuteBond.LockTo function reverts when it is called with an amount <= 52 Wei.

While in the MuteBond contract an attacker can actively make this situation occur and cause a temporary DOS, this is not possible in the MuteAmplifier contract.

The MuteAmplifier contract makes two calls to MuteBond.LockTo:

Link

if (reward > 0) { uint256 week_time = 60 * 60 * 24 * 7; IDMute(dToken).LockTo(reward, week_time ,msg.sender);

userClaimedRewards[msg.sender] = userClaimedRewards[msg.sender].add(
    reward
);
totalClaimedRewards = totalClaimedRewards.add(reward);


emit Payout(msg.sender, reward, remainder);

} Link

if (reward > 0) { uint256 week_time = 1 weeks; IDMute(dToken).LockTo(reward, week_time ,msg.sender);

userClaimedRewards[msg.sender] = userClaimedRewards[msg.sender].add(
    reward
);
totalClaimedRewards = totalClaimedRewards.add(reward);

} In theory there exists the possibility that the rewards that are paid out to a user are > 0 Wei and <= 52 Wei.

If at the endTime this is the case, the rewards will not increase anymore, making it impossible for the staker to withdraw his staked funds, which results in a complete loss of funds.

However with any reasonable value of totalRewards this is not going to occur. Actually it's a real challenge to make the contract output a reward of > 0 Wei and <= 52 Wei.

It might be beneficial to implement the following changes just to be safe:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol index 9c6fcb5..37adc7f 100644 --- a/contracts/amplifier/MuteAmplifier.sol +++ b/contracts/amplifier/MuteAmplifier.sol @@ -242,7 +242,7 @@ contract MuteAmplifier is Ownable{ IERC20(muteToken).transfer(treasury, remainder); } // payout rewards

•    if (reward > 0) {
  

•    if (reward > 52) {
         uint256 week_time = 60 * 60 * 24 * 7;
         IDMute(dToken).LockTo(reward, week_time ,msg.sender);
  

@@ -284,7 +284,7 @@ contract MuteAmplifier is Ownable{ IERC20(muteToken).transfer(treasury, remainder); } // payout rewards

•    if (reward > 0) {
  

•    if (reward > 52) {
         uint256 week_time = 1 weeks;
         IDMute(dToken).LockTo(reward, week_time ,msg.sender);
  

In case rewards are <= 52 Wei they will be lost. But they are worthless anyway.

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L473-L499 https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L366-L388 https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L417-L460

Vulnerability details

Impact

This report deals with how the calculation of the multiplier in the MuteAmplifier contract is not only different from how it is displayed in the documentation on the website but it is also different in a very important way.

The calculation on the website shows a linear relationship between the dMUTE / poolSize ratio and the APY. The dMUTE / poolSize ratio is also called the tokenRatio.
By "linear" I mean that when a user increases his tokenRatio from 0 to 0.1 this has the same effect as when increasing it from 0.9 to 1.

The implementation in the MuteAmplifier.calculateMultiplier function does not have this linear relationship between tokenRatio and APY.
An increase in the tokenRatio from 0 to 0.1 is worth much less than an increase from 0.9 to 1.

As we will see this means that all stakers that do not have a tokenRatio of exactly equal 0 or exactly equal 1 lose out on rewards that they should receive according to the documentation.

I estimate this to be of "High" severity because the issue affects nearly all stakers and results in a partial loss of rewards.

Proof of Concept

Let's first look at the multiplier calculation from the documentation:

The example calculation shows that the amount that is added to $APY_{base}$ (5%) is scaled linearly by the $\dfrac{user_{dmute}}{pool_{rewards}}$ ratio which I called tokenRatio above.

This means that when a user increases his tokenRatio from say 0 to 0.1 he gets the same additional reward as when he increases the tokenRatio from say 0.9 to 1.

Let's now look at how the reward and thereby the multiplier is calculated in the code.

The first step is to calculate the multiplier which happens in the MuteAmplifier.calculateMultiplier function:

Link

function calculateMultiplier(address account, bool enforce) public view returns (uint256) {
    require(account != address(0), "MuteAmplifier::calculateMultiplier: missing account");


    uint256 accountDTokenValue;


    // zkSync block.number = L1 batch number. This at times is the same for a few minutes. To avoid breaking the call to the dMute contract
    // we take the previous block into account
    uint256 staked_block =  _userStakedBlock[account] == block.number ? _userStakedBlock[account] - 1 : _userStakedBlock[account];


    if(staked_block != 0 && enforce)
        accountDTokenValue = IDMute(dToken).getPriorVotes(account, staked_block);
    else
        accountDTokenValue = IDMute(dToken).getPriorVotes(account, block.number - 1);


    if(accountDTokenValue == 0){
        return _stakeDivisor;
    }


    uint256 stakeDifference = _stakeDivisor.sub(10 ** 18);


    // ratio of dMute holdings to pool
    uint256 tokenRatio = accountDTokenValue.mul(10**18).div(totalRewards);


    stakeDifference = stakeDifference.mul(clamp_value(tokenRatio, 10**18)).div(10**18);


    return _stakeDivisor.sub(stakeDifference);
}

The multiplier that is returned is then used to calculate the reward:

Link

reward = lpTokenOut.mul(_totalWeight.sub(_userWeighted[account])).div(calculateMultiplier(account, true));

Let's write the formula in a more readable form:

$\dfrac{lpTokenOut * weightDifference}{stakeDivisor - tokenRatio * (stakeDivisor - 1)}$

$stakeDivisor$ can be any number $>=1$ and has the purpose of determining the percentage of rewards a user with $tokenRatio=0$ gets.

For the sake of this argument we can assume that all numbers except $tokenRatio$ are constant.

Let's just say $stakeDivisor=2$ which means that a user with $tokenRatio=0§ would receive $\dfrac{1}{2}=50%$ of the maximum rewards.

Further let's say that $lpTokenOut * weightDifference = 1$, so 100% of the possible rewards would be $1$.

We can then write the formula like this:

$\dfrac{1}{2 - tokenRatio}$

So let's compare the calculation from the documentation with the calculation from the code by looking at a plot:

x-axis: tokenRatio
y-axis: percentage of maximum rewards

We can see that the green curve is non-linear and below the blue curve.

So the rewards as calculated in the code are too low.

Tools Used

VSCode

Recommended Mitigation Steps

I recommend to change the reward calculation to this:

$(lpTokenOut * weightDifference) * (percentage_{min} + clamp(\dfrac{user_{dmute}}{pool_{rewards}},1) * (1 - percentage_{min}))$

Instead of setting the stakeDivisor upon initialization, the percentageMin should be set which can be in the interval [0,1e18].

Fix:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..1c86f5c 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -48,7 +48,7 @@ contract MuteAmplifier is Ownable{
 
     uint256 private _mostRecentValueCalcTime; // latest update modifier timestamp
 
-    uint256 public _stakeDivisor; // divisor set in place for modification of reward boost
+    uint256 public _percentageMin; // minimum percentage set in place for modification of reward boost
 
     uint256 public management_fee; // lp withdrawal fee
     address public treasury; // address that receives the lp withdrawal fee
@@ -131,8 +131,8 @@ contract MuteAmplifier is Ownable{
      *  @param _mgmt_fee uint
      *  @param _treasury address
      */
-    constructor (address _lpToken, address _muteToken, address _dToken, uint256 divisor, uint256 _mgmt_fee, address _treasury) {
-        require(divisor >= 10 ** 18, "MuteAmplifier: invalid _stakeDivisor");
+    constructor (address _lpToken, address _muteToken, address _dToken, uint256 percentageMin, uint256 _mgmt_fee, address _treasury) {
+        require(_percentageMin <= 10 ** 18, "MuteAmplifier: invalid _percentageMin");
         require(_lpToken != address(0), "MuteAmplifier: invalid lpToken");
         require(_muteToken != address(0), "MuteAmplifier: invalid muteToken");
         require(_dToken != address(0), "MuteAmplifier: invalid dToken");
@@ -142,7 +142,7 @@ contract MuteAmplifier is Ownable{
         lpToken = _lpToken;
         muteToken = _muteToken;
         dToken = _dToken;
-        _stakeDivisor = divisor;
+        _percentageMin = percentageMin;
         management_fee = _mgmt_fee; //bps 10k
         treasury = _treasury;
 
@@ -368,7 +368,7 @@ contract MuteAmplifier is Ownable{
         require(lpTokenOut > 0, "MuteAmplifier::_applyReward: no coins staked");
 
         // current rewards based on multiplier
-        reward = lpTokenOut.mul(_totalWeight.sub(_userWeighted[account])).div(calculateMultiplier(account, true));
+        reward = lpTokenOut.mul(_totalWeight.sub(_userWeighted[account])).div(10 ** 18).mul(calculateMultiplier(account, true)).div(10 ** 18);
         // max possible rewards
         remainder = lpTokenOut.mul(_totalWeight.sub(_userWeighted[account])).div(10**18);
         // calculate left over rewards
@@ -442,7 +442,7 @@ contract MuteAmplifier is Ownable{
             uint256 _totalWeightFee1Local = _totalWeightFee1.add(fee1.mul(10**18).div(totalCurrentStake));
 
             // current rewards based on multiplier
-            info.currentReward = totalUserStake(user).mul(totWeightLocal.sub(_userWeighted[user])).div(info.multiplier_last);
+            info.currentReward = totalUserStake(user).mul(totWeightLocal.sub(_userWeighted[user])).div(10 ** 18).mul(info.multiplier_last).div(10 ** 18);
             // add back any accumulated rewards
             info.currentReward = info.currentReward.add(_userAccumulated[user]);
 
@@ -452,7 +452,7 @@ contract MuteAmplifier is Ownable{
 
         } else {
           // current rewards based on multiplier
-          info.currentReward = totalUserStake(user).mul(_totalWeight.sub(_userWeighted[user])).div(info.multiplier_last);
+          info.currentReward = totalUserStake(user).mul(_totalWeight.sub(_userWeighted[user])).div(10 ** 18).mul(info.multiplier_last).div(10 ** 18);
           // add back any accumulated rewards
           info.currentReward = info.currentReward.add(_userAccumulated[user]);
         }
@@ -485,17 +485,17 @@ contract MuteAmplifier is Ownable{
           accountDTokenValue = IDMute(dToken).getPriorVotes(account, block.number - 1);
 
         if(accountDTokenValue == 0){
-          return _stakeDivisor;
+          return _percentageMin;
         }
 
-        uint256 stakeDifference = _stakeDivisor.sub(10 ** 18);
+        uint256 percentageDifference = (uint256(10 ** 18)).sub(_percentageMin);
 
         // ratio of dMute holdings to pool
         uint256 tokenRatio = accountDTokenValue.mul(10**18).div(totalRewards);
 
-        stakeDifference = stakeDifference.mul(clamp_value(tokenRatio, 10**18)).div(10**18);
+        uint256 additionalPercentage = percentageDifference.mul(clamp_value(tokenRatio, 10**18)).div(10**18);
 
-        return _stakeDivisor.sub(stakeDifference);
+        return _percentageMin.add(additionalPercentage);
     }

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L180-L194

Vulnerability details

Impact

The MuteAmplifier.rescueTokens function allows the owner to withdraw tokens that are not meant to be in this contract.

The contract does protect tokens that ARE meant to be in the contract by not allowing them to be transferred:

Link

function rescueTokens(address tokenToRescue, address to, uint256 amount) external virtual onlyOwner nonReentrant {
    if (tokenToRescue == lpToken) {
        require(amount <= IERC20(lpToken).balanceOf(address(this)).sub(_totalStakeLpToken),
            "MuteAmplifier::rescueTokens: that Token-Eth belongs to stakers"
        );
    } else if (tokenToRescue == muteToken) {
        if (totalStakers > 0) {
            require(amount <= IERC20(muteToken).balanceOf(address(this)).sub(totalRewards.sub(totalClaimedRewards)),
                "MuteAmplifier::rescueTokens: that muteToken belongs to stakers"
            );
        }
    }


    IERC20(tokenToRescue).transfer(to, amount);
}

You can see that lpToken and muteToken cannot be transferred unless there is an excess amount beyond what is needed by the contract.

So stakers can be sure that not even the contract owner can mess with their stakes.

The issue is that lpToken and muteToken are not the only tokens that need to stay in the contract.

There is also the fee0 token and the fee1 token.

So what can happen is that the owner can withdraw fee0 or fee1 tokens and users cannot payout rewards or withdraw their stake because the transfer of fee0 / fee1 tokens reverts due to the missing balance.

Users can of course send fee0 / fee1 tokens to the contract to restore the balance. But this is not intended and certainly leaves the user worse off.

Proof of Concept

Assume that when an update occurs via the update modifier there is an amount of fee0 tokens claimed:

Link

    modifier update() {
        if (_mostRecentValueCalcTime == 0) {
            _mostRecentValueCalcTime = firstStakeTime;
        }


        uint256 totalCurrentStake = totalStake();


        if (totalCurrentStake > 0 && _mostRecentValueCalcTime < endTime) {
            uint256 value = 0;
            uint256 sinceLastCalc = block.timestamp.sub(_mostRecentValueCalcTime);
            uint256 perSecondReward = totalRewards.div(endTime.sub(firstStakeTime));


            if (block.timestamp < endTime) {
                value = sinceLastCalc.mul(perSecondReward);
            } else {
                uint256 sinceEndTime = block.timestamp.sub(endTime);
                value = (sinceLastCalc.sub(sinceEndTime)).mul(perSecondReward);
            }


            _totalWeight = _totalWeight.add(value.mul(10**18).div(totalCurrentStake));


            _mostRecentValueCalcTime = block.timestamp;


            (uint fee0, uint fee1) = IMuteSwitchPairDynamic(lpToken).claimFees();


            _totalWeightFee0 = _totalWeightFee0.add(fee0.mul(10**18).div(totalCurrentStake));
            _totalWeightFee1 = _totalWeightFee1.add(fee1.mul(10**18).div(totalCurrentStake));


            totalFees0 = totalFees0.add(fee0);
            totalFees1 = totalFees1.add(fee1);
        }


        _;
    }

We can see that _totalWeightFee0 is updated such that when a user's rewards are calculated the fee0 tokens will be paid out to the user.

What happens now is that the owner calls rescueTokens which transfers the fee0 tokens out of the contract.

We can see that when the fee0 to be paid out to the user is calculated in the _applyReward function, the calculation is solely based on _totalWeightFee0 and does not take into account if the fee0 tokens still exist in the contract.

Link

fee0 = lpTokenOut.mul(_totalWeightFee0.sub(_userWeightedFee0[account])).div(10**18);

So when the fee0 tokens are attempted to be transferred to the user that calls payout or withdraw, the transfer reverts due to insufficient balance.

Tools Used

VSCode

Recommended Mitigation Steps

The MuteAmplifier.rescueTokens function should check that only excess fee0 / fee1 tokens can be paid out. Such that tokens that will be paid out to stakers need to stay in the contract.

Fix:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..b154d81 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -188,6 +188,18 @@ contract MuteAmplifier is Ownable{
                     "MuteAmplifier::rescueTokens: that muteToken belongs to stakers"
                 );
             }
+        } else if (tokenToRescue == address(IMuteSwitchPairDynamic(lpToken).token0())) {
+            if (totalStakers > 0) {
+                require(amount <= IERC20(IMuteSwitchPairDynamic(lpToken).token0()).balanceOf(address(this)).sub(totalFees0.sub(totalClaimedFees0)),
+                    "MuteAmplifier::rescueTokens: that token belongs to stakers"
+                );
+            }
+        } else if (tokenToRescue == address(IMuteSwitchPairDynamic(lpToken).token1())) {
+            if (totalStakers > 0) {
+                require(amount <= IERC20(IMuteSwitchPairDynamic(lpToken).token1()).balanceOf(address(this)).sub(totalFees1.sub(totalClaimedFees1)),
+                    "MuteAmplifier::rescueTokens: that token belongs to stakers"
+                );
+            }
         }
 
         IERC20(tokenToRescue).transfer(to, amount);

The issue discussed in this report also ties in with the fact that the fee0 <= totalFees0 && fee1 <= totalFees1 check before transferring fee tokens always passes. It does not prevent the scenario that the sponsor wants to prevent which is when there are not enough fee tokens to be transferred the transfer should not block the function.

So in addition to the above changes I propose to add these changes as well:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..39cd75b 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -255,7 +255,7 @@ contract MuteAmplifier is Ownable{
         }
 
         // payout fee0 fee1
-        if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
+        if ((fee0 > 0 || fee1 > 0) && fee0 < IERC20(IMuteSwitchPairDynamic(lpToken).token0()).balanceOf(address(this)) && fee1 < IERC20(IMuteSwitchPairDynamic(lpToken).token1()).balanceOf(address(this))) {
             address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
             address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);
 
@@ -295,7 +295,7 @@ contract MuteAmplifier is Ownable{
         }
 
         // payout fee0 fee1
-        if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
+        if ((fee0 > 0 || fee1 > 0) && fee0 < IERC20(IMuteSwitchPairDynamic(lpToken).token0()).balanceOf(address(this)) && fee1 < IERC20(IMuteSwitchPairDynamic(lpToken).token1()).balanceOf(address(this))) {
             address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
             address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);
 
@@ -331,7 +331,7 @@ contract MuteAmplifier is Ownable{
             }
 
             // payout fee0 fee1
-            if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
+            if ((fee0 > 0 || fee1 > 0) && fee0 < IERC20(IMuteSwitchPairDynamic(lpToken).token0()).balanceOf(address(this)) && fee1 < IERC20(IMuteSwitchPairDynamic(lpToken).token1()).balanceOf(address(this))) {
                 address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(account, fee0);
                 address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(account, fee1);

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L119-L123 https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L153-L200

Vulnerability details

Impact

The maxPayout variable in the MuteBond contract specifies the amount of MUTE that is paid out in one epoch before the next epoch is entered.

The variable is initialized in the constructor and can then be changed via the setMaxPayout function.

The issue occurs when maxPayout is lowered.

So say maxPayout is currently 10,000 MUTE and the owner wants to reduce it to 5,000 MUTE.

Before this transaction to lower maxPayout is executed, another transaction might be executed which increases the current payout to > 5,000 MUTE.

This means that calls to MuteBond.deposit revert and no new epoch can be entered. Thereby the MuteBond contracts becomes unable to provide bonds.

The DOS is not permanent. The owner can increase maxPayout such that the current payout is smaller than maxPayout again and the contract will work as intended.

So the impact is a temporary DOS of the MuteBond contract.

The issue can be solved by requiring in the setMaxPayout function that maxPayout must be bigger than the payout in the current epoch.

Proof of Concept

Add the following test to the bonds.ts test file:

it('POC maxPayout below current payout causes DOS', async function () {
    // owner wants to set maxPayout to 9 * 10**18 
    // However a transaction is executed first that puts the payout in the current epoch above that value
    // all further deposits revert

    // make a payout
    await bondContract.connect(owner).deposit(new BigNumber(10).times(Math.pow(10,18)).toFixed(), owner.address, false)
    
    // set maxPayout below currentPayout
    await bondContract.connect(owner).setMaxPayout(new BigNumber(9).times(Math.pow(10,18)).toFixed())

    // deposit reverts due to underflow
    await bondContract.connect(owner).deposit("0", owner.address, true)
  })

Tools Used

VSCode

Recommended Mitigation Steps

I recommend that the setMaxPayout function checks that maxPayout is set to a value bigger than the payout in the current epoch:

diff --git a/contracts/bonds/MuteBond.sol b/contracts/bonds/MuteBond.sol
index 96ee755..4af01d7 100644
--- a/contracts/bonds/MuteBond.sol
+++ b/contracts/bonds/MuteBond.sol
@@ -118,6 +118,7 @@ contract MuteBond {
      */
     function setMaxPayout(uint _payout) external {
         require(msg.sender == customTreasury.owner());
+        require(_payout > terms[epoch].payoutTotal, "_payout too small");
         maxPayout = _payout;
         emit MaxPayoutChanged(_payout);
     }

Mute Low Risk and Non-Critical Issues

Summary

[L-01] Use fixed compiler version

All in scope contracts use ^0.8.0 as compiler version.
They should use a fixed version, i.e. 0.8.12, to make sure the contracts are always compiled with the intended version.

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L2

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L2

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/dao/dMute.sol#L2

[L-02] RedeemEvent emits wrong to address

In the dMute.RedeemTo function the RedeemEvent is emitted (Link).

It is defined as:
Link

event RedeemEvent(address to, uint256 unlockedAmount, uint256 burnAmount);

So the first parameter should be the to address.

When the event is emitted the first parameter is msg.sender. The issue is that the to address and msg.sender can be different. So the event in some cases contains wrong information.

Fix:

diff --git a/contracts/dao/dMute.sol b/contracts/dao/dMute.sol
index 59f95b7..98a65d5 100644
--- a/contracts/dao/dMute.sol
+++ b/contracts/dao/dMute.sol
@@ -125,7 +125,7 @@ contract dMute is dSoulBound {
         _burn(msg.sender, total_to_burn);
 
 
-        emit RedeemEvent(msg.sender, total_to_redeem, total_to_burn);
+        emit RedeemEvent(to, total_to_redeem, total_to_burn);
     }
 
     function GetUserLockLength(address account) public view returns (uint256 amount) {

[L-03] Replicate price checks from constructor in setter functions

In the MuteBond constructor it is checked that maxPrice >= startPrice (Link).

These checks are not implemented in the setStartPrice and setMaxPrice setter functions.

It is recommended to add the check to both setter functions such that it is ensured the setter functions do not cause startPrice and maxPrice to be set to bad values.

Fix:

diff --git a/contracts/bonds/MuteBond.sol b/contracts/bonds/MuteBond.sol
index 96ee755..49b87f9 100644
--- a/contracts/bonds/MuteBond.sol
+++ b/contracts/bonds/MuteBond.sol
@@ -98,6 +98,7 @@ contract MuteBond {
      */
     function setMaxPrice(uint _price) external {
         require(msg.sender == customTreasury.owner());
+        require(_price >= startPrice, "starting price < min");
         maxPrice = _price;
         emit MaxPriceChanged(_price);
     }
@@ -108,6 +109,7 @@ contract MuteBond {
      */
     function setStartPrice(uint _price) external {
         require(msg.sender == customTreasury.owner());
+        require(maxPrice >= _price, "starting price < min");
         startPrice = _price;
         emit StartPriceChanged(_price);
     }

[L-04] Ownable: Does not implement 2-Step-Process for transferring ownership

MuteAmplifier inherits from the Ownable contract.
This contract does not implement a 2-Step-Process for transferring ownership.
So ownership of the contract can easily be lost when making a mistake when transferring ownership.

Consider using the Ownable2Step contract from OZ (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol) instead.

[L-05] Check that staking cannot occur when endTime is reached

The MuteAmplifier.stake function should require that the current timestamp is smaller than endTime even when the call to stake is the first that ever occurred.
Currently the check is only made in the case that the call to stake is not the first.
The check should be made in both cases.
This is because when staking occurs when block.timestamp >= endTime, no rewards will be paid out. Additionally the user needs to pay the management fee on his LP token stake. So there is really no point in allowing users to do it because it only hurts them.

Fix:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..460c408 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -202,13 +202,12 @@ contract MuteAmplifier is Ownable{
      */
     function stake(uint256 lpTokenIn) external virtual update nonReentrant {
         require(lpTokenIn > 0, "MuteAmplifier::stake: missing stake");
+        require(block.timestamp < endTime, "MuteAmplifier::stake: staking is over");
         require(block.timestamp >= startTime && startTime !=0, "MuteAmplifier::stake: not live yet");
         require(IERC20(muteToken).balanceOf(address(this)) > 0, "MuteAmplifier::stake: no reward balance");
 
         if (firstStakeTime == 0) {
             firstStakeTime = block.timestamp;
-        } else {
-            require(block.timestamp < endTime, "MuteAmplifier::stake: staking is over");
         }
 
         lpToken.safeTransferFrom(msg.sender, address(this), lpTokenIn);

[L-06] Only allow rescuing MUTE rewards when endTime is reached

The MuteAmplifier.rescueTokens function allows the owner to rescue any tokens from the contract.
In the case of MUTE it is checked if totalStakers > 0, i.e. if there are any stakers.
If there are stakers then only tokens in excess of rewards can be rescued.
If there are no stakers, all MUTE tokens can be rescued.
I argue that this behavior does not what is intended. The issue is that there might just temporarily be no stakers but the endTime is not reached yet. This means the contract should be able to payout rewards.

A user that stakes when there are no MUTE rewards (there must still be a small excess balance of MUTE, e.g. sent by an attacker with griefing intent, to pass this check in the stake function) must send MUTE to the contract in order to be able to withdraw again. Otherwise an amount of MUTE is attempted to be transferred that is not held in the contract.

Based on the limited privileges the owner has I don't think the behavior described above is what is intended.

So I recommend that the rescueTokens function should not allow rescuing MUTE rewards within the startTime and endTime at all.

Fix:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..55ee81b 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -183,7 +183,7 @@ contract MuteAmplifier is Ownable{
                 "MuteAmplifier::rescueTokens: that Token-Eth belongs to stakers"
             );
         } else if (tokenToRescue == muteToken) {
-            if (totalStakers > 0) {
+            if (block.timestamp >= startTime && startTime !=0 && block.timestamp < endTime) {
                 require(amount <= IERC20(muteToken).balanceOf(address(this)).sub(totalRewards.sub(totalClaimedRewards)),
                     "MuteAmplifier::rescueTokens: that muteToken belongs to stakers"
                 );

Note: I submitted a similar report that deals with rescuing fee tokens as "Medium" severity. I did this because in the case of rescuing fee tokens it affects EXISTING stakers. Here it affects only stakers that stake AFTER the tokens have been rescued.

[L-07] First user that stakes again after a period without stakers receives too many rewards

The MuteAmplifier contract pays out rewards on a per second basis.
Let's assume there is only 1 staker which is Bob.

Say Bob calls stake at timestamp 0 and calls withdraw at timestamp 10. He receives rewards for 10 seconds of staking.

At timestsamp 30 Bob calls stake again (there were no stakers from timestamp 10 to timestamp 30).
If Bob calls withdraw at say timestamp 40, he receives not only rewards for the 10 seconds he has staked but for 30 seconds (timestamp 10 to timestamp 40).

This means that whenever there are temporarily no stakers, whoever first stakes again receives all the rewards from the previous period without stakers.

This is due to how the update modifier works.

When someone stakes and there were no other stakers, the if block is not entered and the _mostRecentValueCalcTime variable is not updated.

So when the update modifier is executed again the staker also receives the rewards from the period when there were no stakers.

I just want to make the sponsor aware of this behavior. The sponsor may decide that this is unintended and needs to change. I think this might even be a beneficial behavior because it incentivises users to stake if there are no stakers because they will get more rewards.

[L-08] dripInfo function reverts when firstStakeTime >= endTime

It is unlikely but possible that firstStakeTime >= endTime.
I suggest in [L-05] that staking should only occur when block.timestamp < endTime which would mitigate this issue as well. But for now this issue exists.

So when firstStakeTime >= endTime, the following line in the dripInfo function reverts:

Link

info.perSecondReward = totalRewards.div(endTime.sub(firstStakeTime));

The current behavior can cause DOS issues in any components that make use of this function.

I recommend to either implement the changes in [L-05] or to implement the following:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..415da7f 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -416,7 +416,11 @@ contract MuteAmplifier is Ownable{
      */
     function dripInfo(address user) external view returns (DripInfo memory info) {
 
-        info.perSecondReward = totalRewards.div(endTime.sub(firstStakeTime));
+        if (endTime > firstStakeTime) {
+            info.perSecondReward = totalRewards.div(endTime.sub(firstStakeTime));
+        } else {
+            info.perSecondReward = 0;
+        }
         info.totalLP = _totalStakeLpToken;
         info.multiplier_current = calculateMultiplier(user, false);
         info.multiplier_last = calculateMultiplier(user, true);

[L-09] dripInfo function does not calculate fee0 and fee1 in the else block

In the else block, the MuteAmplifier.dripInfo function does not calculate the value for info.fee0 and info.fee1.

Fix:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..20974b8 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -455,6 +455,9 @@ contract MuteAmplifier is Ownable{
           info.currentReward = totalUserStake(user).mul(_totalWeight.sub(_userWeighted[user])).div(info.multiplier_last);
           // add back any accumulated rewards
           info.currentReward = info.currentReward.add(_userAccumulated[user]);
+
+          info.fee0 = totalUserStake(user).mul(_totalWeightFee0.sub(_userWeightedFee0[user])).div(10**18);
+          info.fee1 = totalUserStake(user).mul(_totalWeightFee1.sub(_userWeightedFee1[user])).div(10**18);
         }
 
     }

[L-10] It is possible in theory that stakes get locked due to call to LockTo with very small reward amount

I pointed out and explained in my report #7 MuteBond.sol: deposit function reverts if remaining payout is very small due to >0 check in dMute.LockTo function how the MuteBond.LockTo function reverts when it is called with an amount <= 52 Wei.

While in the MuteBond contract an attacker can actively make this situation occur and cause a temporary DOS, this is not possible in the MuteAmplifier contract.

The MuteAmplifier contract makes two calls to MuteBond.LockTo:

Link

if (reward > 0) {
    uint256 week_time = 60 * 60 * 24 * 7;
    IDMute(dToken).LockTo(reward, week_time ,msg.sender);


    userClaimedRewards[msg.sender] = userClaimedRewards[msg.sender].add(
        reward
    );
    totalClaimedRewards = totalClaimedRewards.add(reward);


    emit Payout(msg.sender, reward, remainder);
}

Link

if (reward > 0) {
    uint256 week_time = 1 weeks;
    IDMute(dToken).LockTo(reward, week_time ,msg.sender);


    userClaimedRewards[msg.sender] = userClaimedRewards[msg.sender].add(
        reward
    );
    totalClaimedRewards = totalClaimedRewards.add(reward);
}

In theory there exists the possibility that the rewards that are paid out to a user are > 0 Wei and <= 52 Wei.

If at the endTime this is the case, the rewards will not increase anymore, making it impossible for the staker to withdraw his staked funds, which results in a complete loss of funds.

However with any reasonable value of totalRewards this is not going to occur. Actually it's a real challenge to make the contract output a reward of > 0 Wei and <= 52 Wei.

It might be beneficial to implement the following changes just to be safe:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..37adc7f 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -242,7 +242,7 @@ contract MuteAmplifier is Ownable{
           IERC20(muteToken).transfer(treasury, remainder);
         }
         // payout rewards
-        if (reward > 0) {
+        if (reward > 52) {
             uint256 week_time = 60 * 60 * 24 * 7;
             IDMute(dToken).LockTo(reward, week_time ,msg.sender);
 
@@ -284,7 +284,7 @@ contract MuteAmplifier is Ownable{
           IERC20(muteToken).transfer(treasury, remainder);
         }
         // payout rewards
-        if (reward > 0) {
+        if (reward > 52) {
             uint256 week_time = 1 weeks;
             IDMute(dToken).LockTo(reward, week_time ,msg.sender);

In case rewards are <= 52 Wei they will be lost. But they are worthless anyway.

[L-11] Check for each fee token individually that non-zero value is transferred

The MuteAmplifier contract executes the following code when fee tokens are transferred:

Link

if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
    address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
    address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);

So when one of the fee tokens has a value that is greater 0, both fee tokens are transferred. This means that it is possible for a transfer to occur with the zero value.

There exist tokens that revert on zero-value transfer. Also the upstream contract that transfers the fee tokens to the MuteAmplifier contract checks for each token individually that the amount is greater zero:

Link

function claimFeesFor(address recipient, uint amount0, uint amount1) external {
    require(msg.sender == pair);
    if (amount0 > 0) _safeTransfer(token0, recipient, amount0);
    if (amount1 > 0) _safeTransfer(token1, recipient, amount1);
}

Therefore I recommend to check in the MuteAmplifier contract the value for each token individually as well.

Fix:

diff --git a/contracts/amplifier/MuteAmplifier.sol b/contracts/amplifier/MuteAmplifier.sol
index 9c6fcb5..42b4ec2 100644
--- a/contracts/amplifier/MuteAmplifier.sol
+++ b/contracts/amplifier/MuteAmplifier.sol
@@ -256,8 +256,8 @@ contract MuteAmplifier is Ownable{
 
         // payout fee0 fee1
         if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
-            address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
-            address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);
+            if (fee0 > 0) address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
+            if (fee1 > 0) address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);
 
             totalClaimedFees0 = totalClaimedFees0.add(fee0);
             totalClaimedFees1 = totalClaimedFees1.add(fee1);
@@ -296,8 +296,8 @@ contract MuteAmplifier is Ownable{
 
         // payout fee0 fee1
         if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
-            address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
-            address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);
+            if (fee0 > 0) address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
+            if (fee1 > 0) address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);
 
             totalClaimedFees0 = totalClaimedFees0.add(fee0);
             totalClaimedFees1 = totalClaimedFees1.add(fee1);
@@ -332,8 +332,8 @@ contract MuteAmplifier is Ownable{
 
             // payout fee0 fee1
             if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
-                address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(account, fee0);
-                address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(account, fee1);
+                if (fee0 > 0) address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(account, fee0);
+                if (fee1 > 0) address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(account, fee1);
 
                 totalClaimedFees0 = totalClaimedFees0.add(fee0);
                 totalClaimedFees1 = totalClaimedFees1.add(fee1);

[N-01] Remove require statements that are always true

The following two require statements always pass:

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/dao/dMute.sol#L99-L100

lock_info.amount and lock_info.tokens_minted are of type uint256 so they cannot be < 0.
In fact the check that tokens_to_mint > 0 in the LockTo function even ensures that lock_info.amount and lock_info.tokens_minted are greater 0.

[N-02] Remove SafeMath library

All 3 in-scope contracts use the SafMath library for simple addition, subtraction, multiplication and division.

This causes an unnecessary overhead since beginning from Solidity version 0.8.0 arithemtic operations revert by default on overflow / underflow.

By looking into the implementation of the SafeMath library you can also see that it is just a wrapper around the basic arithmatic operations and does not add any checks (at least for the functions used in the in-scope contracts) (Link).

[N-03] Event parameter names are messed up

In the MuteBond contract, the following events are defined:

Link

event BondCreated(uint deposit, uint payout, address depositor, uint time);
event BondPriceChanged(uint internalPrice, uint debtRatio);
event MaxPriceChanged(uint _price);
event MaxPayoutChanged(uint _price);
event EpochDurationChanged(uint _payout);
event BondLockTimeChanged(uint _duration);
event StartPriceChanged(uint _lock);

Some of the parameter names do not accurately describe the variable that is actually emitted. E.g. for the EpochDurationChanged event, it is the new epoch duration that is emitted and not a _payout variable.

There is also an inconsistency with the MaxPayoutChanged and StartPriceChanged events. So I recommend to use more descriptive names in the event emissions for these 3 events.

[N-04] Event is never emitted

There are two events defined that are never emitted. They can be removed to make the code cleaner.

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L13

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L22

[N-05] Move payoutFor calculation into else block

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L155

In case the if block is entered, payout is calculated twice.
Therefore the payoutFor call can be moved into the else block to clean up the code and save a little bit of Gas.

Fix:

diff --git a/contracts/bonds/MuteBond.sol b/contracts/bonds/MuteBond.sol
index 96ee755..b462992 100644
--- a/contracts/bonds/MuteBond.sol
+++ b/contracts/bonds/MuteBond.sol
@@ -152,11 +152,12 @@ contract MuteBond {
      */
     function deposit(uint value, address _depositor, bool max_buy) external returns (uint) {
         // amount of mute tokens
-        uint payout = payoutFor( value );
+        uint payout;
         if(max_buy == true){
           value = maxPurchaseAmount();
           payout = maxDeposit();
         } else {
+          payout = payoutFor( value );
           // safety checks for custom purchase
           require( payout >= ((10**18) / 100), "Bond too small" ); // must be > 0.01 payout token ( underflow protection )
           require( payout <= maxPayout, "Bond too large"); // size protection because there is no slippage

[N-06] Remove redundant check in stake function

The following check is redundant:

Link

require(IERC20(muteToken).balanceOf(address(this)) > 0, "MuteAmplifier::stake: no reward balance");

This check is redundant because before this check there is another check that startTime!=0 which means the initializeDeposit function has been called which ensures the MUTE balance is not zero.

There are edge cases where the current check would apply, e.g. when staking occurs after the endTime. But the current check is not sufficient in this case because there could just be a little excess MUTE balance in the contract and the user would still not get rewards. So I recommend to remove the existing check and the edge cases will be addressed by the other changes I propose in this report.