An attacker can front-run setMaxPayout() and freeze deposit() and the whole protocol from progressing in epochs.

[code423n4]

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

[c4-judge]

Picodes marked the issue as duplicate of #35

[Picodes]

The severity here is overinflated to me as the report does not highlight the fact that the owner can just set back the value to solve the issue.

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes marked the issue as satisfactory