code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/src/PuttyV2.sol#L610-L614

Vulnerability details

Impact

Invalid contract addresses are allowed

Proof of Concept

function _transferERC721sIn(ERC721Asset[] memory assets, address from) internal { for (uint256 i = 0; i < assets.length; i++) { ERC721(assets[i].token).safeTransferFrom(from, address(this), assets[i].tokenId); } }

Tools Used

VS code

Recommended Mitigation Steps

for ERC20 there is validation check like the below one, function _transferERC20sIn(ERC20Asset[] memory assets, address from) internal { for (uint256 i = 0; i < assets.length; i++) { address token = assets[i].token; uint256 tokenAmount = assets[i].tokenAmount;

        require(token.code.length > 0, "ERC20: Token is not contract");
        require(tokenAmount > 0, "ERC20: Amount too small");

        ERC20(token).safeTransferFrom(from, address(this), tokenAmount);
    }
}

similarly, contract address validation can be added for ERC721

rotcivegaf commented 2 years ago

If there are some invalid contract address(ERC20 or ERC721), the transaction revert when try call safeTransferFrom to the invalid contract addresses

GalloDaSballo commented 2 years ago

Invalid contract addresses are allowed, and the transaction will most likely fail, even in the case of a malicious token, the counter-party (the other side of the order) could still verify that there's a malicious token

outdoteth commented 2 years ago

For completeness, the code check is applied in ERC20 transfers because solmate's low level transfer call does not apply this check themselves:

require(token.code.length > 0, "ERC20: Token is not contract");

It's not necessary for a regular external contract interaction (via ERC721.safeTransferFrom) because this will revert if the contract does not exist.

HickupHH3 commented 1 year ago

Agreed that validation of ERC721 isn't required here

code-423n4 / 2022-06-putty-findings