code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/auction/Auction.sol#L363

Vulnerability details

ERC20 transfer with not checked return value

Impact

Not every ERC20 token follows OpenZeppelin's recommendation. It's possible (inside ERC20 standard) that a transfer() doesn't revert upon failure but returns false.

The code doesn't check these return values. In case the assembly call fails for transfering ETH, meaning success := false

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

Then, the _handleOutgoingTransfer would enter the block that uses transfer with not checked return value:

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

            // Transfer WETH instead
            IWETH(WETH).transfer(_to, _amount);
        }

Several tokens do not return a boolean on these functions. WETH token is not verified to have the returning bool value. As a result, their calls in the contract might fail.

Proof of Concept

https://github.com/code-423n4/2022-09-nouns-builder/blob/debe9b792cc70510eadf9b3728cde5b0f2ec9a1f/src/auction/Auction.sol#L363

Recommended Mitigation Steps

Consider using OpenZeppelin's library with safe versions of transfer functions. Check return value.

GalloDaSballo commented 2 years ago

We know that WETH will always return true