code-423n4 / 2022-09-nouns-builder-findings

10 stars 6 forks source link

Unchecked return value of WETH.transfer can allow to bid for free #669

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

In Auction._handleOutgoingTransfer, the return value of IWETH(WETH).transfer() is not checked. This would allow failed WETH transfer to be ignored and allow bidder to bid without making payment or for treasury to not be transferred ETH to.

Proof of Concept

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

Tools Used

Manual review

Recommended Mitigation Steps

Add a require check for successful transfer.

zobront commented 2 years ago

How exactly would someone have a failed WETH transfer that wouldn't revert? From WETH source code:

    function transfer(address dst, uint wad) public returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }

    function transferFrom(address src, address dst, uint wad)
        public
        returns (bool)
    {
        require(balanceOf[src] >= wad);

        if (src != msg.sender && allowance[src][msg.sender] != uint(-1)) {
            require(allowance[src][msg.sender] >= wad);
            allowance[src][msg.sender] -= wad;
        }

        balanceOf[src] -= wad;
        balanceOf[dst] += wad;

        Transfer(src, dst, wad);

        return true;
    }

I don't believe this is exploitable, given that the token will always be WETH and not an arbitrary ERC20.

GalloDaSballo commented 2 years ago

Dup of https://github.com/code-423n4/2022-09-nouns-builder-findings/issues/293