Mute Switch - Versus contest - 0xA5DF's results

Lines of code

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

Vulnerability details

The MuteBond.deposit() function allows users to specify the amount of value they want to purchase bonds for or to set max_buy to true. If max_buy is set to true the amount specified in the value parameter is ignored and instead the maximum amount available for purchase in the current epoch is used. This can lead to a scenario where a user intends to purchase the remaining amount of current epoch, but till the tx is included in the blockchain a new epoch starts (either by an innocent user or by an attacker) and the user ends up buying the entire amount of the next epoch.

Impact

A. The user ends up buying a much higher amount than intended B. The user ends up buying it for a lower price than intended (i.e. less payout for the buyer)

Proof of Concept

The PoC below shows how maxPurchaseAmount() increases when a new era starts.

File: test/bonds.ts

  it('Max buy PoC', async function () {

    // buy 99% of amount available for purchase in current epoch
    let maxValue = await bondContract.maxPurchaseAmount();
    let depositValue = maxValue.mul(99).div(100);
    await bondContract.connect(buyer1).deposit(depositValue, buyer1.address, false);
    
    // The amount available when the victim sends out the tx
    var expectedDeposit = await bondContract.maxPurchaseAmount()

    await bondContract.connect(buyer1).deposit('0', buyer1.address, true);

    // The amount available when the victims's tx is included in the blockchain
    var actualDeposit = await bondContract.maxPurchaseAmount();    

    // expected deposit = 1 wad
    // actual deposit = 100 wad
    console.log({expectedDeposit, actualDeposit});
  })

The following snippet shows that when a user sets max_buy to true the value used is the maxPurchaseAmount()

        if(max_buy == true){
          value = maxPurchaseAmount();
          payout = maxDeposit();
        } else {

Recommended Mitigation Steps

Require the user to specify the epoch number when doing a 'max buy', and revert if it doesn't match the current epoch (it might be a good idea to refactor the code to 2 external functions for normal buy and max buy, where they both share an internal function to make the actual deposit)

Side note: this is similar to another bug I've reported regarding getting a lower price than expected, however the root cause, impact, and mitigation are different and therefore I've reported this separately.

Lines of code

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

Vulnerability details

The MuteBond contract contains a feature in which after each purchase the epochStart increases by 5% of the time passed since epochStart, this (in most cases) lowers the bond's price (i.e. buyer gets less payout) for future purchases. An attacker can exploit this feature to front-run a deposit/purchase tx and lower the victim's payout. This can also happen by innocent users purchasing before the victim's tx is included in the blockchain.

Another (less likely) scenario in which this can happen is when the owner changes the config in a way that lowers the price (e.g. lowering max price, extending epoch duration), if the owner tx executes while a user's deposit() tx is in the mempool the user would end up with less payout than intended.

Side note: the term 'bond price' might be confusing since it refers to the payout the buyer gets divided by the value the buyer pays, so a higher price is actually in favor of the buyer.

Impact

User ends up buying bond for a lower payout than intended.

Proof of Concept

In the PoC below, an attacker manages to make the buyer purchase a bond at a price lower by 32% than intended.

File: test/bonds.ts

  it('Front run PoC', async function () {
    // let price reach the max price
    await time.increase(60 * 60 * 24 * 7)

    // price when victim sends out the tx to the mempool
    var expectedPrice = await bondContract.bondPrice()

    const startPrice = new BigNumber(100).times(Math.pow(10,18))
    let minPurchasePayout = new BigNumber(Math.pow(10,16));
    // using dynamic price didn't work out so I'm using the lowest price
    var minPurchaseValue = minPurchasePayout.times(1e18).div(startPrice).plus(1);

    // attacker buys the lowest amount 20 times
    for(let i = 0; i< 20; i++){
      await bondContract.connect(buyer1).deposit(minPurchaseValue.toFixed(), buyer1.address, false)
    }

    var init_dmute = await dMuteToken.GetUnderlyingTokens(buyer1.address)
    let depositValue = new BigNumber(10).times(Math.pow(10,18)).toFixed();
    var price = await bondContract.connect(buyer1).deposit(depositValue, buyer1.address, false)
    var post_dmute = await dMuteToken.GetUnderlyingTokens(buyer1.address)

    var dmute_diff = new BigNumber(post_dmute.toString()).minus(init_dmute.toString());
    var actualPrice = dmute_diff.times(1e18).div(depositValue);

    var receipt = (await price.wait())
    // compare the expected price with the actual price
    // expected price = 200; actual price = 135.8; meaning actual price is ~68% of expected price
    console.log({expectedPrice, actualPrice:actualPrice.toString()});
  })

Recommended Mitigation Steps

Add a min payout parameter so that users can specify the expected payout. The tx should revert if the actual payout is lower than expected.

Lines of code

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

Vulnerability details

The amplifier's APY is calculated based on the user's dMute balance (delegation balance to be more accurate) - the more dMute the user holds the higher APY they get. However, the contract only checks the user's dMute balance at staking, the user doesn't have to hold that balance at any time but at the staking block.

This let's the user to bribe somebody to delegate them their dMute for a single block and stake at the same time.

Since the balance checked is the delegation balance (getPriorVotes) this even makes it easier - since the 'lender' doesn't even need to trust the 'borrower' to return the funds, all the 'lender' can cancel the delegation on their own on the next block.

The lender can also create a smart contract to automate and decentralize this 'service'.

Impact

Users can get a share of the rewards that are supposed to incentivize them to hold dMute without actually holding them. In other words, the same dMute tokens can be used to increase simultaneous staking of different users.

Proof of Concept

The following code shows that only a single block is being checked to calculate the accountDTokenValue at calculateMultiplier():

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

The block checked when rewarding is the staked block, since enforce is true when applying reward:

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

Recommended Mitigation Steps

Make sure that the user holds the dMute for a longer time, one way to do it might be to sample a few random blocks between staking and current blocks and use the minimum balance as the user's balance.

Lines of code

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

Vulnerability details

The maxPayout variable can be changed by the owner at any time. In case the owner lowers the maxPayout and the payoutTotal of the current epoch is greater than the new maxPayout the contract will be broken - no further deposit can be made, and most of the view functions will be broken too. This can be done either by an attacker front running the owner's call, by an innocent user, or by the owner not noticing the payoutTotal is already high.

Impact

The MuteBonds contract will be unavailable for some time.

This can be patched by the owner increasing it back to the original value, however the owner might be a multisig or a governance token which takes some time to pass a new vote / sign it all. Additionally, lowering maxPayout back as intended might be complicated, esp. if there's an attacker involved.

Proof of Concept

File: test/bonds.ts

  it('Max buy PoC', async function () {

    // buy 99% of max payout
    let maxValue = await bondContract.maxPurchaseAmount();
    let maxPayout = await bondContract.maxPayout();
    let depositValue = maxValue.mul(99).div(100);
    await bondContract.connect(buyer1).deposit(depositValue, buyer1.address, false);

    // owner lowers it max payout to 98% of current max payout 
    let newMaxPayout = maxPayout.mul(98).div(100);
    await bondContract.connect(owner).setMaxPayout(newMaxPayout);

    // `maxDeposit()` will revert, since deposit depends on it it'll revert too 
    try {
      maxValue = await bondContract.maxDeposit();
    } catch (e) {
      console.log(e);
    }


  })

Recommended Mitigation Steps

At setMaxPayout() check if the total payout of the current epoch is equal-or-greater than the new max payout and start a new era in that case.

Bonds might have a very small amount left to end the current era

There might be a case where a user bought a bond in an amount that leaves the contract a very low (up to 1 unit) amount away from ending an era. In that case it's still possible to end the era using max_buy, but it might not be worth the gas fee to purchase such a low amount.

As mitigation - end the era when the total payout of the current era is close enough to the max payout, even if it's not the exact amount.

bondPrice() naming might be confusing

The name 'bond price' is a bit confusing since the user that calls deposit() is considered to be the bond buyer (according to the docs), meaning a higher price is actually in favor of the buyer (they get a higher payout). Therefore, it might be better to change the name to a more clear name (e.g. 'LP price' since it indicates the payout the user gets per LP).

Remove SafeMath library

Gas saved: 2.1K for each tx that uses SafeMath + 100 units per call.

It seems like safe math doesn't serve any purpose here and it can simply be replaced with normal math operands. The usage of library is pretty expensive since it makes a delegate call to an external address.

Make MuteAmplifier variables immutable when possible

Gas saved: up to 20K per tx

The following variables can be immutable:

• lpToken
• muteToken
• dToken
• _stakeDivisor
• management_fee
• treasury

Refactor initializeDeposit() into the constructor to make more variables immutable

Gas saved: ~7K per tx The logic of initializeDeposit() cen be integrated into the constructor to make the following variables immutable:

• totalRewards
• startTime
• endTime

The only issue is sending tokens before the contract creation. This can be solved in one of the following ways:

• Pre-calculate the address of the contract and approve the tokens to the address before creating it
• Add a deployer contract that will create the MuteAmplifier contract. The deployer contract will hold the tokens and will contain a callback that when called would send the tokens to the sender. The callback will revert if it'll be called at any time except during deployment (it'll use a storage variable to indicate if we're during a deployment or not)

Use lock_index to look for empty elements

Gas saved: 2.2K * (amount of non-empty elements)

At dMute.redeemTo() the function iterates through the entire array to find empty elements, this means the entire array is being read in search for the empty elements. Instead, we can find the empty elements using the lock_index and iterate through them only.

At MuteAmplifier.update() skip updating if done in the same block

Gas saved: A few thousands

When update() is being called a second time in the same block both the _totalWeight and _mostRecentValueCalcTime don't really change. But the logic to update them still run and uses a few thousands of gas units. Skipping that when sinceLastCalc is zero can save that gas.

Additionally, the remaining logic regarding fees might be skipped too (not sure about that).

Anyways, if the fee0 or fee1 are zero, some of the logic can be skipped too and save gas.

MuteBond.deposit() makes an unnecessary call to payoutFor() during max buy

Gas saved: a few hundreds probably

In the following code the payoutFor() is unnecessary when max_buy is true and therefore should be inside the else block.

        uint payout = payoutFor( value );
        if(max_buy == true){
          value = maxPurchaseAmount();
          payout = maxDeposit();
        } else {

Storage variable caching

Gas saved: 100 per cache

The following variables can be cached into memory instead of being read (or written) twice:

Here 2 storage variables are being read twice: amplifier/MuteAmplifier.sol#L370-L373

        // current rewards based on multiplier
        reward = lpTokenOut.mul(_totalWeight.sub(_userWeighted[account])).div(calculateMultiplier(account, true));
        // max possible rewards
        remainder = lpTokenOut.mul(_totalWeight.sub(_userWeighted[account])).div(10**18);

_userStakedBlock is being read twice here: amplifier/MuteAmplifier.sol#L480-L481

        uint256 staked_block =  _userStakedBlock[account] == block.number ? _userStakedBlock[account] - 1 : _userStakedBlock[account];

epochStart is being read and written a few times: bonds/MuteBond.sol#L185-L197

        // 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){
            terms.push(BondTerms(0,0,0));
            epochStart = block.timestamp;
            epoch++;
        }

_applyReward() doing sstores twice when called from stake()

Gas saved: ~400

When _applyReward() is being called from stake() the following lines are writing to variables that are then written again at _stake():

        _totalStakeLpToken = _totalStakeLpToken.sub(lpTokenOut);

        _userStakedLpToken[account] = 0;

        _userAccumulated[account] = 0;

Rewriting _applyReward() so that it won't do those changes (e.g. pass a parameter to indicate whether it's being called from stake() or not) and instead doing them at _stake() can save gas.

Replace string errors with custom errors

Gas saved per instance: Probably a few thousands for deployment + a few hundreds when the function reverts

Custom errors are cheaper both for deployment and reverts, since strings take up lots of bytes code while custom reverts just a single byte.

MuteBond's epoch() and currentEpoch() are the same

Gas saved: a few units per tx

epoch() and currentEpoch() are basically the same, each public/external function adds a conditional check for each tx (since the contract compares the call data against the function has to find the right one). Removing this duplication can save a few gas units.

Unnecessary multiplication and division by 1e18

Gas saved: ~20 units (200+ units when using SafeMath)

In the following line the multiplication and division by 1e18 is unnecessary. Instead, just make sure all multiplications are done before division, that will prevent rounding in the same manner.

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