No slippage control for deposit() with the impact that a user deposits with expected high bond price might end up a deposit with the lowest bond price.

[code423n4]

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.

[c4-judge]

Picodes marked the issue as duplicate of #24

[chaduke3730]

This attack is different from #24: #24 exploits the fact that the price will be decreased after each deposit; This attack exploits the fact that a new epoch will start from the lowest price. This attacks tries to push a staker to the new epoch, as a result, the stakers ends up the lowest price instead of the highest price of the last epoch.

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #25

[c4-judge]

Picodes marked the issue as not a duplicate

[c4-judge]

Picodes marked the issue as duplicate of #9

[Picodes]

@chaduke3730 I respectfully disagree. Even if the scenario you're describing is indeed different, the root issue is the same: the bond price can change over time and there is no way for users to protect themselves against a price drop.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[chaduke3730]

I respectfully accept your decision.

[chaduke3730]

Will all my dups of 24 consolidated and counted as one dup of 24?