Mute Switch - Versus contest - hansfriese's results

Lines of code

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

Vulnerability details

Impact

Users might deposit more LPs unexpectedly if a malicious user increases an epoch by frontrunning.

Proof of Concept

deposit() has a max_buy param to purchase all remaining amounts.

    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
        }
        ...
    }

But this function is frontrunable and the below situation would be possible.

1. Currently, maxPayout = 1000, epoch = 1, maxDeposit() = 100. An honest user called deposit() with max_buy = true to purchase bonds for 100 payouts.
2. After noticing this, a malicious user frontruns deposit() with max_buy = true, and the epoch was increased to 2 now. Without the malicious user, it would be possible during the normal purchases.
3. So for the honest user, epoch = 2, payout = maxDeposit() = 10000 and it will try to transfer LP tokens for 10000 payouts from the user.
4. If the honest user doesn't have enough LP balance or didn't approve that amount, the transfer will revert which is not so bad.
5. But if the honest user has enough balance and approved type(uint256).max like this test, he will be suffered to use more LPs than his expectations.

Tools Used

Manual Review

Recommended Mitigation Steps

I think deposit() should have an epoch or max_value_to_pay param to protect users when max_buy = true.

And it should revert if the epoch was increased or the final value is greater than the upper limit of value(max_value_to_pay).

Lines of code

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

Vulnerability details

Impact

There will be 2 impacts.

• The reward system would be broken as the rewards can be withdrawn before starting staking.
• Some rewards would be locked inside the contract forever as it doesn't check totalReclaimed

Proof of Concept

rescueTokens() checks the below condition to rescue muteToken.

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

But there are 2 problems.

1. Currently, it doesn't check anything when totalStakers == 0. So some parts(or 100%) of rewards can be withdrawn before the staking period. In this case, the reward system won't work properly due to the lack of rewards.
2. It checks the wrong condition when totalStakers > 0 as well. As we can see here, some remaining rewards are tracked using totalReclaimed and transferred to treasury directly. So we should consider this amount as well.

Tools Used

Manual Review

Recommended Mitigation Steps

It should be modified like the below.

else if (tokenToRescue == muteToken) {
    if (totalStakers > 0) { //should check totalReclaimed as well
        require(amount <= IERC20(muteToken).balanceOf(address(this)).sub(totalRewards.sub(totalClaimedRewards).sub(totalReclaimed)),
            "MuteAmplifier::rescueTokens: that muteToken belongs to stakers"
        );
    }
    else if(block.timestamp <= endTime) { //no stakers but staking is still active, should maintain totalRewards
        require(amount <= IERC20(muteToken).balanceOf(address(this)).sub(totalRewards),
            "MuteAmplifier::rescueTokens: that muteToken belongs to stakers"
        );
    }
}

Lines of code

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

Vulnerability details

Impact

Users can stake after endTime due to the wrong check.

Proof of Concept

When a user stakes LP tokens using MuteAmplifier.stake, stake is not allowed after endTime which is set in initializeDeposit by an admin.

    require(block.timestamp < endTime, "MuteAmplifier::stake: staking is over");

But it doesn't check for the first stake, so the first stake can be done after the endTime and it is not correct.

Tools Used

Manual Review

Recommended Mitigation Steps

We should check the endTime for the first stake.

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

Lines of code

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

Vulnerability details

Impact

rescueTokens() can be used to withdraw fee tokens without any validations.

As a result, the reward logic would be broken due to the lack of fee tokens.

Proof of Concept

rescueTokens() doesn't validate anything for the fee tokens.

So if some fee tokens were withdrawn, the reward withdrawal logic will revert as the contract doesn't have enough balance.

File: MuteAmplifier.sol
257:         // payout fee0 fee1
258:         if ((fee0 > 0 || fee1 > 0) && fee0 <= totalFees0 && fee1 <= totalFees1) {
259:             address(IMuteSwitchPairDynamic(lpToken).token0()).safeTransfer(msg.sender, fee0);
260:             address(IMuteSwitchPairDynamic(lpToken).token1()).safeTransfer(msg.sender, fee1);
261: 
262:             totalClaimedFees0 = totalClaimedFees0.add(fee0);
263:             totalClaimedFees1 = totalClaimedFees1.add(fee1);
264: 
265:             emit FeePayout(msg.sender, fee0, fee1);
266:         }

Tools Used

Manual Review

Recommended Mitigation Steps

rescueTokens() should validate for the fee tokens as well.

    function rescueTokens(address tokenToRescue, address to, uint256 amount) external virtual onlyOwner nonReentrant {
        address token0 = IMuteSwitchPairDynamic(lpToken).token0();
        address token1 = IMuteSwitchPairDynamic(lpToken).token1();

        ...
        ...
        else if (tokenToRescue == token0) {
            require(amount <= IERC20(token0).balanceOf(address(this)).sub(totalFees0.sub(totalClaimedFees0)),
                "MuteAmplifier::rescueTokens: that token0 belongs to stakers"
            );
        }
        else if (tokenToRescue == token1) {
            require(amount <= IERC20(token1).balanceOf(address(this)).sub(totalFees1.sub(totalClaimedFees1)),
                "MuteAmplifier::rescueTokens: that token1 belongs to stakers"
            );
        }

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

Lines of code

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

Vulnerability details

Impact

The Mutebond contract might stop working after the owner decreased maxPayout by a malicious frontrunner.

Proof of Concept

setMaxPayout() can be used to reset maxPayout.

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

And in deposit(), epoch is increased only when terms[epoch].payoutTotal == maxPayout.

File: MuteBond.sol
193:         if(terms[epoch].payoutTotal == maxPayout){
194:             terms.push(BondTerms(0,0,0));
195:             epochStart = block.timestamp;
196:             epoch++;
197:         }

So when the owner is going to decrease maxPayout for some reason, the below scenario would be possible by a malicious user.

1. Currently, epoch = 1, maxPayout = 1000, terms[epoch].payoutTotal = 500
2. The owner decided to decrease maxPayout to 800 for some reason and called setMaxPayout() with _payout = 800.
3. After noticing it, a malicious user frontruns deposit() with value = 400.
4. After deposit() is called, terms[epoch].payoutTotal = 900 and maxPayout will be 800.
5. Then that, deposit() will revert because maxDeposit() reverts due to uint underflow.

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

As a result, the contract doesn't work until the owner increases back maxPayout.

It would be more serious when it takes certain time to execute the admin functions as it should be approved by DAO.

Tools Used

Manual Review

Recommended Mitigation Steps

setMaxPayout() should check if the updated maxPayout isn't less than the current epoch's payoutTotal.

    function setMaxPayout(uint _payout) external {
        require(msg.sender == customTreasury.owner());
        require(_payout >= terms[epoch].payoutTotal); //++++++++++++++++++++
        
        maxPayout = _payout;
        emit MaxPayoutChanged(_payout);
    }

[L-01] MuteAmplifier.rescueTokens() should check pre/post balances for 2 addresses tokens

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

It's safe to check pre/post balances for 2 addresses tokens.

[L-02] It checks the wrong conditions for fee transfers

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

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

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

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

    totalClaimedFees0 = totalClaimedFees0.add(fee0);
    totalClaimedFees1 = totalClaimedFees1.add(fee1);

    emit FeePayout(msg.sender, fee0, fee1);
}

When it checks for fee payouts, it doesn't consider totalClaimedFees0/1 at all. It should be checked like the below.

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

    totalClaimedFees0 = totalClaimedFees0.add(fee0);
    totalClaimedFees1 = totalClaimedFees1.add(fee1);

    emit FeePayout(msg.sender, fee0, fee1);
}

[L-03] MuteBond.setMaxPrice() and MuteBond.setStartPrice() should check if maxPrice >= startPrice

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

    function setMaxPrice(uint _price) external {
        require(msg.sender == customTreasury.owner());
        maxPrice = _price;
        emit MaxPriceChanged(_price);
    }

    /**
     *  @notice sets the start price for the LP token
     *  @param _price uint
     */
    function setStartPrice(uint _price) external {
        require(msg.sender == customTreasury.owner());
        startPrice = _price;
        emit StartPriceChanged(_price);
    }

When the above functions are called by the owner, maxPrice >= startPrice should be checked like here.

Otherwise, this one will revert during deposit.

[L-04] MuteBond.setEpochDuration() should check if epochDuration > 0

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

epochDuration should be greater than 0, otherwise, this line will revert during deposit.

[L-05] MuteBond.setStartPrice() should check if startPrice > 0

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

If startPrice = 0, bondPrice might be 0 at the start time of epoch and this line will revert during deposit.

[L-06] Should check > instead of >=

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

    require(lock_info.amount >= 0 , "dMute::Redeem: INSUFFICIENT_AMOUNT");
    require(lock_info.tokens_minted >= 0 , "dMute::Redeem: INSUFFICIENT_MINT_AMOUNT");

Currently, the above conditions will be true with the empty info. There is no serious impact but it will work without reverting for the wrong indices. It should use > instead.