c4-bot-2 commented 8 months ago

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L195 https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L426-L442

Vulnerability details

Impact

The AuctionHouse.createBid function is used to create a bid for a Verb, with a given amount. In this function if a new bid is placed the previous bidder gets the refund of his bid amount as shown below:

    if (lastBidder != address(0)) _safeTransferETHWithFallback(lastBidder, _auction.amount);

The AuctionHouse._safeTransferETHWithFallback function as its name suggests has a fallback mechanism. If the transfer Eth transaction using the low-level call function is reverted then the Eth amount is converted to WETH and transferred to the previous bidder as ERC20 token. This implementation is shown below:

    assembly {
        // Transfer ETH to the recipient
        // Limit the call to 50,000 gas
        success := call(50000, _to, _amount, 0, 0, 0, 0)
    }

    // If the transfer failed:
    if (!success) {
        // Wrap as WETH
        IWETH(WETH).deposit{ value: _amount }();

        // Transfer WETH instead
        bool wethSuccess = IWETH(WETH).transfer(_to, _amount);

        // Ensure successful transfer
        if (!wethSuccess) revert("WETH transfer failed");
    }

But the issue is if the previous bidder is a smart contract which can not handle the ERC20 tokens (Eg : not being able to transfer the WETH token to another address or unable to withdraw the WETH and retrieve the ETH amount) then the WETH token will be locked in the previous bidder contract and as a result the bidder will lose his funds which he spent for the previous bid.

Proof of Concept

        if (lastBidder != address(0)) _safeTransferETHWithFallback(lastBidder, _auction.amount);

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L195

        assembly {
            // Transfer ETH to the recipient
            // Limit the call to 50,000 gas
            success := call(50000, _to, _amount, 0, 0, 0, 0)
        }

        // If the transfer failed:
        if (!success) {
            // Wrap as WETH
            IWETH(WETH).deposit{ value: _amount }();

            // Transfer WETH instead
            bool wethSuccess = IWETH(WETH).transfer(_to, _amount);

            // Ensure successful transfer
            if (!wethSuccess) revert("WETH transfer failed");
        }

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L426-L442

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

It is recommended, in the event of a fallback to store the converted WETH amount in the AuctionHouse contract for the bidder in a mapping with the bidder address and the bid amount. So the respective bidder can later send a signed transaction to the AcutionHouse with a correct recipient address (which can handle WETH token) which the auction house can verify msg.sender to be the bidder address and then transfer the WETH funds to the correct recipient address which can handle the WETH tokens. This way the previous bidder can retrieve his funds safely. The signed signature handling functionality should be implemented in the AuctionHouse.sol contract.

Or else it is recommended to implement a getWethBid function (the mapping between bidder address and bid amount is used to find the transfer bid weth amount for the calling bidder) which is callable by the previous bidder to receive the WETH tokens from the AuctionHouse.sol contract.

Assessed type

Other

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as duplicate of #10

c4-judge commented 8 months ago

MarioPoneder changed the severity to QA (Quality Assurance)