code423n4 commented 3 years ago

Handle

kenzo

Vulnerability details

A malicious user can listen to the mempool and immediately bond when an auction starts, without aim of settling the auction. As no one can cancel his bond in less than 24h, this will freeze user funds and auction settlement for 24h until his bond is burned and the new index is deleted. The malicious user can then repeat this when a new auction starts.

Impact

Denial of service of the auction mechanism. The malicious user can hold the basket "hostage" and postpone or prevent implementing new index. The only way to mitigate it would be to try to front-run the malicious user, obviously not ideal.

Proof of Concept

publishAllIndex: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L170

The attacker would listen to this function / PublishedNewIndex event and upon catching it, immediately bond the auction.
The publisher has no way to burn a bond before 24h has passed. But even if he could, it would not really help as the attacker could just bond again (though losing funds in the process). settleAuction: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L79
Only the bonder can settle. bondBurn: https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L111
Can only burn 24h after bond.

Tools Used

Manual analysis, hardhat.

Recommended Mitigation Steps

If we only allow one user to bond, I see no real way to mitigate this attack, because the malicious user could always listen to the mempool and immediately bond when an auction starts and thus lock it. So we can change to a mechanism that allows many people to bond and only one to settle; but at that point, I see no point to the bond mechanism any more. So we might as well remove it and let anybody settle the auction.

With the bond mechanism, a potential settler would have 2 options:

Bond early: no one else will be able to bond and settle, but the user would need to leave more tokens in the basket (as newRatio starts large and decreases in time)
Bond late: the settler might make more money as he will need to leave less tokens in the basket, but he risks that somebody else will bond and settle before him. Without a bond mechanism, the potential settler would still have these equivalent 2 options:
Settle early: take from basket less tokens, but make sure you win the auction
Settle late: take from basket more tokens, but risk that somebody settles before you So that's really equivalent to the bonding scenario.

I might be missing something but at the moment I see no detriment to removing the bonding mechanism.

frank-beard commented 3 years ago

https://github.com/code-423n4/2021-09-defiprotocol-findings/issues/276

GalloDaSballo commented 2 years ago

I think the fundamental issue here is that there can only be one bonder that rebalances, an elegant solution would be to move to a mapping of bonders that rebalance.

Allowing anyone to bond and rebalance while still allowing the same dynamic of burning their bonds

This would be more in line with the idea of offering bonds for discounts (Olympus Pro) to anyone as long as there's enough underlying

I agree with the finding and believe the severity is appropriate as this effectively slows down the rebalancing by at least 2 days

code-423n4 / 2021-09-defiprotocol-findings