Incorrect mapping key in withdrawAllFailedCredits allows attackers to steal other users failed transfer credits.

Description

• When _payout function is triggered and the transfer of the funds fails for some reason, failedTransferCredits mapping is introduced to keep track of funds that should have been sent to the user. User can then try again to retrieve those funds that failed to be transferred to him by using function withdrawAllFailedCredits.

• Problem lies in the inconsistency of verification to whom those funds belong and applying proper access control. Malicious actor can supply the address of the user that has some balance saved inside of the failedTransferCredits mapping, but instead of eth being sent to the user that should actually receive it and properly handling the state of mapping failedTransferCredits , eth is sent to the msg.sender (which in this case is malicious actor) instead of the supplied _receiver address. Since the mapping is updated with the msg.sender key instead of _receiver , malicious actor can call this function multiple times resulting in draining the protocol out of all the eth balance.

Risk

Likelihood:

• This is possible anytime there is an element in the failedTransferCredits mapping that is holding the amount that is greater than 0, the only way for this mapping to be populated is through finding a way to call _payout function and failing the transfer of eth inside it.

• Anyone can call the withdrawAllFailedCredits function and supply the address of choice since the function is external and has no implemented access control.

Impact:

• Exploiting this vulnerability results in direct loss of the protocols eth.

Proof of Concept

Inside of the src/BidBeastsMarketPlaceTest.t.sol , i created this Attack contract. The purpose of this contract is to automate the process of calling the withdrawAllFailedCredits function multiple times to drain as much funds as possible. Custom logic is implemented in receive function that checks if the target balance is big enough to attempt the stealing of eth and then attempting to do it.

Here is the actual function that show the path of exploiting this vulnerability, first there is a simulation of creating listings and bidding so that the protocol would have some eth on its balance. Then the rejector contract is used to make a bid on one listing, but that bid is intentionally bigger than the max price because it will trigger the 'buy now' logic that sends the excess eth back to the rejector. It is important to note that rejector has no implemented fallback or receive function resulting in a failure of receiving eth and thus setting up the failedTransferCredits mapping for exploitation. It is enough for attacker to call withdrawAllFailedCredits once because that function will then be called multiple times by going through the receive logic inside of the attacker contract - bouncing back and forth and resulting in draining of protocols funds.

Recommended Mitigation

There are multiple ways of solving this problem, one way is to change the function as shown in below example that will allow anyone to call it but the mapping will be updated properly and the eth would be send to the proper address.

The other way to fix this is instead of supplying other users address, users can withdraw only for himself and thus again resulting in the proper receival of eth as well as the proper update of mapping.