code423n4 commented 1 year ago

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