code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Vulnerability Details

It was observed that the PuttyV2 contract uses solmate's SafeTransferLib for pulling ERC20 assets from the order maker or taker to the PuttyV2 contract.

There is some difference between the implementation of solmate's SafeTransferLib and OZ's SafeERC20:

OZ's SafeERC20 checks if the token is a contract or not, solmate's SafeTransferLib does not.

Per the comment in https://github.com/code-423n4/2022-06-putty/blob/3b6b844bc39e897bd0bbb69897f2deff12dc3893/contracts/lib/solmate/src/utils/SafeTransferLib.sol#L9

/// @dev Note that none of the functions in this library check that a token has code at all! That responsibility is delegated to the caller.

As such, if the token address within the ERC20(token).safeTransferFrom() function call points to an address with zero bytecode (= no code), the transaction will succeed with no error.

Due to this design, a number of issues could happen if token with zero bytecode at the address is being used in an order.

Proof-of-Concept

Following illustrate an example of this vulnerability

Instance #1 - Malicious order maker could trick someone to fulfill the order without paying premium

Alice (Attacker) create an "Long Call" order with a not-yet-exist tokens as its order.baseAsset.
Bob (Victim) filled Alice's order by calling PuttyV2.fillOrder. The function will attempts to transfer the premium from Alice to Bob. However, since order.baseAsset points to an address with zero bytecode, it will silently suceed even though nothing is transferred to Bob.

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

function fillOrder(
    Order memory order,
    bytes calldata signature,
    uint256[] memory floorAssetTokenIds
) public payable returns (uint256 positionId) {
    ..SNIP..
    // transfer premium to whoever is short from whomever is long
    if (order.isLong) {
        ERC20(order.baseAsset).safeTransferFrom(order.maker, msg.sender, order.premium);
    } else {

Once Alice's order has been filled, Alice exercises her "Long Call" option and obtain the ERC20 Token or ERC721 NFT belonging to Bob

Impact

The order taker (victim) did not receive any premium while taking on risk.

Recommendation

It is recommended to use Openzeppelin's SafeERC20 instead. By default, OZ's SafeERC20 will check that there is bytecode at the token address before the transfer.

Alternatively, implement additional check to ensure that there is bytecode at the token address.