Open code423n4 opened 2 years ago
Great find, adding the same bounty multiple times can allow the exploiter to drain the entire contract as long as the bounties are denominated in the underlying token
Because the warden shows a way to cause DOS, which is contingent on setting it as well as redeeming that bounty (which can be avoided by simply not adding it to the bountiesID calldata), I will rate this finding as Medium Severity
The DOS is low severity as it can be sidestep very easily, see #82
However, the warden has identified a technical issue with the protocol (unflagging of bounty), as such I agree with a medium severity
Handle
0xRajeev
Vulnerability details
Impact
The withdrawBounty() loops through the _bounties array looking for active bounties and transferring amounts from active ones. However, the data location specifier used for bounty is memory which makes a copy of the _bounties array member instead of a reference. So when bounty.active is set to false, this is changing only the memory copy and not the array element of the storage variable. This results in bounties never being set to inactive, keeping them always active forever and every withdrawBounty() will attempt to transfer bounty amount from the Auction contract to the msg.sender.
Therefore, while the transfer will work the first time, subsequent attempts to claim this bounty will revert on transfer (because the Auction contract will not have required amount of bounty tokens) causing withdrawBounty() to always revert and therefore preventing settling of any auction.
A malicious attacker can add a tiny bounty on any/every Auction contract to prevent any reindexing on that contract to happen because it will always revert on auction settling. This can be used to cause DoS on any auctionBonder so as to make them lose their bondAmount because their bonded auction cannot be settled.
Proof of Concept
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L143
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Auction.sol#L143-L147
https://docs.soliditylang.org/en/v0.8.7/types.html#data-location-and-assignment-behaviour
Tools Used
Manual Analysis
Recommended Mitigation Steps
Recommend changing storage specifier of bounty to "storage" instead of “memory".