search

code-423n4 / 2023-06-reserve-findings

1 stars 0 forks source link

After trade has finished, BackingManager.rebalance is called with same trade kind #46

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/reserve-protocol/protocol/blob/c4ec2473bbcb4831d62af55d275368e73e16b984/contracts/p1/BackingManager.sol#L89

Vulnerability details

Impact

After trade has finished, BackingManager.rebalance is called with same trade kind. That means that users can't change auction for the chain of trades.

Proof of Concept

BackingManager allows only 1 trade at a time. When trade has finished, then it should be settled, so new trade can be created then.

When you settle the function, then rebalance is called again in order to start next trade that should repair the system. The problem is that same trade kind is passed to the rebalance function as it was used on previous trade. That actually means that if we need to do 10 trades to recollateralize system and we started with Gnosis auction or Dutch auction, then all this 10 trades will be done with same auction type. But sometimes one auction is better than another one(for example gas prices are high or smth else) and user would like to choose which one to start, but they don't have such option and this can have impact on received amount and the system.

Tools Used

VsCode

Recommended Mitigation Steps

Think about how to handle this. Maybe you don't need to call rebalance right after settling.

Assessed type

Error

0xean commented 1 year ago

Will wait for sponsor review, but think QA is appropriate for this.

tbrent commented 1 year ago

We have a preference for Dutch auctions over Batch auctions that makes this QA. Agree that is maybe not sufficiently documented at the moment.

tbrent commented 1 year ago

More on this: calling rebalance() during settlement of the Dutch auction is intended alignment of incentives. If it is not called, then an altruistic actor has to burn gas to call rebalance() for every trade. By having the bidder in the auction pay for this, we can actually have some expectation that it will happen. It is already an unfortunate limitation that the very first rebalance() must be paid for by an altruistic actor.

c4-sponsor commented 1 year ago

tbrent marked the issue as disagree with severity

c4-sponsor commented 1 year ago

tbrent marked the issue as sponsor acknowledged

c4-judge commented 1 year ago

0xean changed the severity to QA (Quality Assurance)