Incorrect data location specifier can be abused to cause DoS and fund loss

[code423n4]

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

[GalloDaSballo]

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

[GalloDaSballo]

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