Mute Switch - Versus contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. The deposit() function will bump bond price back by 5% after purchase based on current delta. However, this function can be executed unlimited number of times in the same block and as a result, one can exploit this vulnerability to attack another depositor: he can lower the price of another depositor() to the lowest by frontrunning.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how an attacker can lower the price of another depositor() by frontrunning:

1. The bondPrice will increase the price of the bond from startPrice to maxPrice linearly in terms of the amount of elapsed time since epochStart.

function bondPrice() public view returns (uint) {
        uint timeElapsed = block.timestamp - epochStart;
        uint priceDelta = maxPrice - startPrice;

        if(timeElapsed > epochDuration)
          timeElapsed = epochDuration;

        return timeElapsed.mul(priceDelta).div(epochDuration).add(startPrice);
    }

2. Meanwhile, the deposit() function will bump bond price back by 5% after purchase based on current delta.

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

3. However, there is no restriction in terms how many times deposit() can be called in one block. So an attacker can call it many times (say 50 times) to lower the price of the bond (0.95^50 = 0.0762).

4. Suppose Alice sees a good price of the bond, so she decides she will buy the bond.

5. Attacker Bob comes a long and call deposit() 50 times (with the minimum 0.01e18 payout) and reduce the price near startPrice() (see implementation of bondPrice()).

6. As a result, Alice purchased the bond with a very lower price and she is not happy with the result.

Tools Used

VSCode

Recommended Mitigation Steps

1. We will make sure within each block the price can be bumped back at most once.

2. We need to have some slippage control for the depositor so that the depositor will get the minium amount of payout for the input lptoken.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. There is a race condition between MuteBond#setEpochDuration() and MuteBond#deposit(). The issue is that when a new EpochDuration is set, it will take effect immediately, which will affect the bond price. As a result, depending on the order of these two functions, users who deposit before or after MuteBond#setEpochDuration() will have two totally different bond prices - a race condition.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Let's analyze a race condition between MuteBond#setEpochDuration() and MuteBond#deposit():

1) ``MuteBond#setEpochDuration()`` will change ``epochDuration``, which will take effect immediately;

2) The bond price will be changed due to the change of ``epochDuration``, see implementation of [BondPrice()](https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L227-L235)

3) The deposit() function will use the BondPrice() to calculate the amount of payout. As a result, depositors right before or after ``MuteBond#setEpochDuration()`` will use two totally bond prices to calculate such payouts. This is unfair to depositors. 

4) When the ``epochDuration`` is prolonged, a user might front-run ``MuteBond#setEpochDuration()`` to get a better bond price (before price), while users who fail to do so have to use the price after ``MuteBond#setEpochDuration()``, a worse price. 

Tools Used

VSCode

Recommended Mitigation Steps

The new set epochDuration should only take effect in the next epoch.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

There is no slippage control for deposit().

Impact: a user deposits with expected high bond price might end up a deposit with the lowest bond price.

Scenario: a depositor waits for the end of an epoch, expecting to enjoy a good price near maxPrice. When he finally calls deposit(), another user front-runs the transaction with deposit() and push the first depositor into a new epoch, with the lowest price for bond, the startPrice. The first depositor ends up a deposit with the lowest price startPrice instead - a big surprise and disappointment.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

1. The deposit() function uses the bondPrice() to calculate the payout for each depositor. The price of the bond will increase from startPrice to maxPrice with slight fallback after each deposit.

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

2. For each epoch, there is a total maxPayout limit, when it is reached, the protocol enters into a new epoch, and the bond price starts from startPrice again.

   if(terms[epoch].payoutTotal == maxPayout){
            terms.push(BondTerms(0,0,0));
            epochStart = block.timestamp;
            epoch++;
        }

3. The problem lies in the possibility of the following front-running: a depositor waits for the end of an epoch, expecting to enjoy a good price near maxPrice. When he finally calls deposit(), another user front-runs the transaction with deposit() and push the first depositor into a new epoch, with the lowest price for bond, the startPrice.

4. While we cannot prevent front-running, we should give the first depositor a slippage control, so that he will not buy the bond with a price lower than he expected. He expected a price near maxPrice, not a price near startPrice.

Tools Used

VSCode

Recommended Mitigation Steps

Add a slippage control to deposit() so that a user will only deposit with the bond price he expected.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. An attacker can launch a DOS attack to RedeemTo() and GetUnderlyingTokens() so that it will always fail for a particular account, say Bob. In this way, Bob will not be able to redeem the MuteToken locked under him. He will lose funds.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how an attacker, Alice, can launch a DOS attack to RedeemTo() and GetUnderlyingTokens():

1. call LockTo(1, 52 weeks, Bob) repeatedly. As a result, the length of the array _userLocks[Bob] will be very large. The attacker only needs to spend 1wei of MuteToken.

2. When Bob calls RedeemTo(), the function will always fail due the the following for-loop as it iterates through each element in the array _userLocks[Bob]. When the length of the array is too large, this loop will run out of gas.

 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();
          }
        }

3. GetUnderlyingTokens(Bob) will always fail due to the large length of the array _userLocks[Bob] since the function needs to iterate through each element in the array. It will run out of gas.

Tools Used

VScode

Recommended Mitigation Steps

1. Use mapping instead of array, avoiding iterating through an array who length can be very large
2. set a minimum lock value to increase the cost for attacker.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. A staker might be still be able to stake after staking is over.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

When nobody stakes during the whole staking period, then the first staker can still stake, even after the staking period is already over.

This is because of the following faulty logic:

 if (firstStakeTime == 0) {
            firstStakeTime = block.timestamp;
        } else {
            require(block.timestamp < endTime, "MuteAmplifier::stake: staking is over");
        }

So when firstStakeTime == 0 and block.timestamp > endTime, it is still possible to stake. In other words, the function never check for the first staker whether the staking is over or not, it always allows the first staker to stake.

Tools Used

VScode

Recommended Mitigation Steps

if (firstStakeTime == 0) {
            firstStakeTime = block.timestamp;
} 
- else {
            require(block.timestamp < endTime, "MuteAmplifier::stake: staking is over");
-        }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. When the owner calls setMaxPayout() to decrease maxPayout to newMaxPayout, an attacker can front-run it and deposit so that terms[epoch].payoutTotal <= maxPayout but terms[epoch].payoutTotal > newMaxPayout. This will freeze deposit() and the whole protocol all together.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Let's see how an attacker can front-run setMaxPayout() to freeze the whole protocol:

1. Suppose we have terms[epoch].payoutTotal = 899,999e18 and maxPayout = 1,000,000e18.

2. Suppose the owner wants to call setMaxPayout(900,000e18) so that maxPayout can be set to 900,000e18.

3. An attacker front-runs the call setMaxPayout(900,000e18) and calls deposit() to deposit with a payout of 2e18. As a result, we have terms[epoch].payoutTotal = 900,001e18.

4. Now setMaxPayout(900,000e18) gets executed, with maxPayout set to 900,000e18. As a result, we have terms[epoch].payoutTotal = 900,001e18 > maxPayout.

5. The deposit() function will always call maxDeposit(), which will always fail due to an underflow:

   function maxDeposit() public view returns (uint) {
        return maxPayout.sub(terms[epoch].payoutTotal);
    }

6. Epoch will never be progressed since the following block inside deposit() will never get executed due to failure of deposit(). Besides, the condition of the if-statement will never be true.

  if(terms[epoch].payoutTotal == maxPayout){
            terms.push(BondTerms(0,0,0));
            epochStart = block.timestamp;
            epoch++;
        }

7. Due to the front-running, deposit() will always fail, no epoch can be progressed, the system is frozen.

Tools Used

VScode

Recommended Mitigation Steps

When maxDeposit() is called, the new maxPayout will only be in effect in the next Epoch:

 function setMaxPayout(uint _payout) external {
        require(msg.sender == customTreasury.owner());
-        maxPayout = _payout;
+        newMaxPayout = _payout;
        emit MaxPayoutChanged(_payout);
    }

function deposit(uint value, address _depositor, bool max_buy) external returns (uint) {
        // amount of mute tokens
        uint payout = payoutFor( value );
        if(max_buy == true){
          value = maxPurchaseAmount();
          payout = maxDeposit();
        } 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, "Bond too large"); // size protection because there is no slippage
          require( payout <= maxDeposit(), "Deposit too large"); // size protection because there is no slippage
        }


        // total debt is increased
        totalDebt = totalDebt.add( value );
        totalPayoutGiven = totalPayoutGiven.add(payout); // total payout increased

        customTreasury.sendPayoutTokens(payout);
        TransferHelper.safeTransferFrom(lpToken, msg.sender, address(customTreasury), value ); // transfer principal bonded to custom treasury

        // indexed events are emitted
        emit BondCreated(value, payout, _depositor, block.timestamp);

        bonds.push(Bonds(value, payout, _depositor, block.timestamp));
        // redeem bond for user, mint dMute tokens for duration of vest period
        IDMute(dMuteToken).LockTo(payout, bond_time_lock, _depositor);

        terms[epoch].payoutTotal = terms[epoch].payoutTotal + payout;
        terms[epoch].bondTotal = terms[epoch].bondTotal + value;
        terms[epoch].lastTimestamp = block.timestamp;

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

        // exhausted this bond, issue new one
        if(terms[epoch].payoutTotal == maxPayout){
+           if(newMaxPayout !=0 ) maxPayout = newMaxPayout; // @audit: there is a change
            terms.push(BondTerms(0,0,0));
            epochStart = block.timestamp;
            epoch++;
+           newMaxPayout = 0;
        }

        return payout;
    }

QA1. payout() lacks the emit of event of emit Payout(msg.sender, reward, remainder);, which is emitted at L245 of the withdraw() function. Without this event, the reward payout cannot be consistently monitored.

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

The fix is as follows:

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);
+           emit Payout(msg.sender, reward, remainder);    
        }

QA2. It is important to add require(_maxPrice >= _startPrice, "starting price < min") to both setMaxPrice() and setStartPrice(). Otherwise, if the constraint is violated, the protocol will break. For example, bondPrice() will always fail.

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

QA3. There is no sanity check for the input of setBondTimeLock(). With inappropriate input, the protocol will break. For example, the timeToTokens() restricts the _lock_time to be between 1 week and 52 weeks. As a result, when _lock is set to be out of this range, the whole protocol will break.

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

Mitigation: to be consistent, introduce the same range check for setBondTimeLock():

function setBondTimeLock(uint _lock) external {
        require(msg.sender == customTreasury.owner());
+       require(_lock >= 1 weeks && _lock <= 52 weeks) revert _lockOutOfRange(); 
        bond_time_lock = _lock;
        emit BondLockTimeChanged(_lock);
    }

QA3. There is a division-before-multiplication precision loss issue with timeToTokens()

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

The problem lies in the following line:

        uint256 base_tokens = _amount.mul(_lock_time.mul(10**18).div(max_lock)).div(10**18);

It has a division first:

_lock_time.mul(10**18).div(max_lock)

which has a precision loss up to max_lock = 3.145e7.

Then multiplication here:

_amount.mul(_lock_time.mul(10**18).div(max_lock))

Mitigation: use multiplication before division as follows:

- uint256 base_tokens = _amount.mul(_lock_time.mul(10**18).div(max_lock)).div(10**18);
+ uint256 base_tokens = _amount.mul(_lock_time).div(max_lock);

QA4: rescueTokens() cannot rescue muteTokens when totalStakers == 0.

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

The if-condition creates the problem:

  if (totalStakers > 0) {
                require(amount <= IERC20(muteToken).balanceOf(address(this)).sub(totalRewards.sub(totalClaimedRewards)),
                    "MuteAmplifier::rescueTokens: that muteToken belongs to stakers"
                );
            }

Mitigation:

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);
    }

G1. Define week_time and max_lock as public private constants to save gas.

https://github.com/code-423n4/2023-03-mute/blob/64521eff60cebca4d7c43f670bdc1414f8704fd0/contracts/dao/dMute.sol#L50-L51

G2. I don't see how multiplication by 1e18 and then division by 1e18 will help minimizing precision loss here. So maybe we can save gas instead:

https://github.com/code-423n4/2023-03-mute/blob/64521eff60cebca4d7c43f670bdc1414f8704fd0/contracts/dao/dMute.sol#L57

-  uint256 base_tokens = _amount.mul(_lock_time.mul(10**18).div(max_lock)).div(10**18);
+  uint256 base_tokens = (_amount.mul(_lock_time)).div(max_lock);

G3. No need to check these conditions, they will always be true. They can be eliminated to save gas.

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

G4. Here we only need to reset the time

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

- _userLocks[msg.sender][index] = UserLockInfo(0,0,0);
+ _userLocks[msg.sender][index].time = 0;

G5. Drop the first condition since it will be always true. https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/amplifier/MuteAmplifier.sol#L139

- require(_mgmt_fee >= 0 && _mgmt_fee <= 1000, "MuteAmplifier: invalid _mgmt_fee");
+ require(_mgmt_fee <= 1000, "MuteAmplifier: invalid _mgmt_fee");

G6. This line can be eliminated since the check will always pass due to L205 and the checks in initializeDeposit() (L166).

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

G7. There is no need for the if-condition since it is always be true due to the check at L231 inside the function _applyReward(). That check will ensure lpTokenOut > 0.

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

G8. This safety check can be eliminated since we can infer epochStart <= block.timestamp will always be true. This is because each time we will only add 5% of the timeElapsed to epochStart. as a result, epochStart will be approaching near block.timestamp but will never exceed it.

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

Consider how epochStart was calculated: The previous two lines show how to calculate epochStart:

G9. fee0 <= totalFees0 && fee1 <= totalFees1 are always going to be true, so there is no need to check them.

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

We can see that these two checks are always be true because totalFees0 and totalFees1 are changed ONLY at lines L116 and L117 of the update() function, and from the way that fee0 and fee1 are obtained in function _applyReward(), fee0 and fee1 will always be a partial amount of totalFees0 and totalFees1. Therefore, the checks will always be true. There is no need to check.

In addition, checking them against balance of the two fees are not necessary either since the safeTransfer() will do that automatically.

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

-            totalClaimedFees0 = totalClaimedFees0.add(fee0);
            totalClaimedFees1 = totalClaimedFees1.add(fee1);
+        }
            emit FeePayout(msg.sender, fee0, fee1);
        }

G10. This line can be eliminated since the next line will check this condition anyway (transferFrom()).

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

G11. One can retrieve the value from info.totalLP to save gas:

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

- uint256 totalCurrentStake = totalStake();
+  uint256 totalCurrentStake = info.totalLP;

G12. We can save gas by avoid using index i-1, use i instead. Note that we know _userLocks[msg.sender].length > 0 for sure otherwise the function would have reverted earlier.

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

-  for(uint256 i = _userLocks[msg.sender].length; i > 0; i--){
+  for(unchecked{uint256 i = _userLocks[msg.sender].length-1}; ; unchecked{i--}){
-          UserLockInfo memory lock_info = _userLocks[msg.sender][i - 1];
+          UserLockInfo memory lock_info = _userLocks[msg.sender][i];


          // 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][i] = _userLocks[msg.sender][_userLocks[msg.sender].length - 1];
            _userLocks[msg.sender].pop();
          }
+         if(i == 0) break;
        }

G13. startTime == 0 has to be true since there is no other places that assign a value to it. No check is needed, the line can be eliminated.

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