code-423n4 / 2023-03-mute-findings

2 stars 1 forks source link

There is a race condition betweeen MuteBond#setEpochDuration() and MuteBond#deposit() #15

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. There is a race condition between MuteBond#setEpochDuration() and MuteBond#deposit(). The issue is that when a new EpochDuration is set, it will take effect immediately, which will affect the bond price. As a result, depending on the order of these two functions, users who deposit before or after MuteBond#setEpochDuration() will have two totally different bond prices - a race condition.

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 analyze a race condition between MuteBond#setEpochDuration() and MuteBond#deposit():

1) ``MuteBond#setEpochDuration()`` will change ``epochDuration``, which will take effect immediately;

2) The bond price will be changed due to the change of ``epochDuration``, see implementation of [BondPrice()](https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/bonds/MuteBond.sol#L227-L235)

3) The deposit() function will use the BondPrice() to calculate the amount of payout. As a result, depositors right before or after ``MuteBond#setEpochDuration()`` will use two totally bond prices to calculate such payouts. This is unfair to depositors. 

4) When the ``epochDuration`` is prolonged, a user might front-run ``MuteBond#setEpochDuration()`` to get a better bond price (before price), while users who fail to do so have to use the price after ``MuteBond#setEpochDuration()``, a worse price. 

Tools Used

VSCode

Recommended Mitigation Steps

The new set epochDuration should only take effect in the next epoch.

c4-sponsor commented 1 year ago

mattt21 marked the issue as sponsor confirmed

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #24

Picodes commented 1 year ago

To me, this is another instance of #24. Delaying the update until the next epoch won't really solve the issue: we could always imagine deposit transactions being pending in the mempool during a whole epoch. The root cause is again that depositors cannot protect themselves against unfavorable price changes.

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

c4-judge commented 1 year ago

Picodes changed the severity to 3 (High Risk)

chaduke3730 commented 1 year ago

I agree with your decision.