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

2 stars 1 forks source link

Owner lowering max payout might break the MuteBonds contract #26

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

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #35

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory