Putty contest - exd0tpy's results

Lines of code

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

Vulnerability details

Impact

If some asset in order.erc20Assets amount is set to zero, user cannot execute _transferERC20sIn

Proof of Concept

There is no check for order.erc20Assets amount.

    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);
        }
    }

If user call _transferERC20sIn when order.erc20Assets has zero amount asset, will revert.

Tools Used

VS Code

Recommended Mitigation Steps

check order.erc20Assets has zero amount asset

using ReentrancyGuard for potential attack

There is user callback in _transferERC721sOut.

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

    function safeTransferFrom(
        address from,
        address to,
        uint256 id
    ) public virtual {
        transferFrom(from, to, id);

        require(
            to.code.length == 0 ||
                ERC721TokenReceiver(to).onERC721Received(msg.sender, from, id, "") ==
                ERC721TokenReceiver.onERC721Received.selector,
            "UNSAFE_RECIPIENT"
        );
    }

Now there is no risk. but using ReentrancyGuard for the future.

Use ++i than i++ in for loop

++i is using less gas than i++