code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

when fillOrder() is called code mints two PuttyV2 NFT token, one for Long position and one for Short Position and It's possible to transfer this NFT tokens to others. exercising unwanted bad Long positions can cause users to lose funds and tokens, for example if someone exercise Long CALL BTC at $100K or Long PUT BTC at $1K s/he would lose lots of funds, so attacker can create lots of Long positions NFTs(he can sign short orders which order.baseAsset is some worthless token and then call fillOrder() to create Long position NFT) and then transfer them to all protocol users (in one transaction to save gas) and users would see them in the UI and if one user by mistake clicks on exercise() (attacker can even create malicious positions based on each user balances and trick them to click on exercise) he would lose his funds because contract would transfer user funds and in return user receive some worthless order.baseAsset tokens.

Proof of Concept

This is transferFrom() code in PuttyV2NFT contract:

    // remove balanceOf modifications
    function transferFrom(
        address from,
        address to,
        uint256 id
    ) public override {
        require(from == _ownerOf[id], "WRONG_FROM");
        require(to != address(0), "INVALID_RECIPIENT");
        require(
            msg.sender == from || isApprovedForAll[from][msg.sender] || msg.sender == getApproved[id],
            "NOT_AUTHORIZED"
        );

        _ownerOf[id] = to;
        delete getApproved[id];

        emit Transfer(from, to, id);
    }

As you can see it allows to transfer NFT position tokens to others. attacker can use this to create a lots of malicious Long positions and transfer them to other users and trick users (or users by mistake) to click on exercise() and steal users funds. To exploit this attacker would look at balance of each user of protocol and create malicious Long positions like Long CALL BTC at $100K or Long PUT BTC at $1K (based on user ERC20 or ERC721 token balances) and transfer them to users. To create Long positions attacker first signs the malicous short position with one of his wallets and set baseAsset to some worthless asset or set Premium to 0 and then by calling fillOrder() with his another wallet attacker would create a lot of Long positions (he can do this in one transaction to save gas) then he would transfer position NFT tokens to others and trick them to click on exercise() on UI (or some user click on it by mistake because of huge number of malicious long NFT positions so some users would do this mistake) and then contract would transfer user funds to contract address and give user some worthless tokens and user would lose his funds and attacker can claim user funds by calling withdraw() for the opposite position(short position).

Tools Used

VIM

Recommended Mitigation Steps

Transferring Long positions shouldn't be allowed because exercising bad long position can cause users to lose funds.

GalloDaSballo commented 2 years ago

You can transfer the ERC721 to anyone, that is true. Why would they exercise the contract though?

outdoteth commented 2 years ago

Questionable if this is even an issue imo. It is desired behaviour that any option can be exercised. And it is quite clear on the platform that it's up to the user's discretion whether they should or shouldn't exercise an option.

A "long BTC put @ $1k" is not "worthless" as is implied in the exploit scenario. That option still has some monetary value. So by transferring it to 1000's of people, the attacker is giving away free money.

HickupHH3 commented 2 years ago

Maybe one can carry out a phishing attack via the path the warden described, like what we recently saw with UniV3, but it would be OOS.

There are too many hypothetical assumptions for this issue to be considered valid. Agree with sponsor that it should be left to the user to decide whether to exercise an option.

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