Contract `PhiNFT1155` can't be paused

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/art/PhiNFT1155.sol#L21-L31

Vulnerability details

Impact

The pausing mechanism of PhiNFT1155 contract is implemented incorrectly and doesn't work: users are still able to transfer NFTs in paused state.

Summary

Contract PhiNFT1155 inherits from the following parent contracts:

contract PhiNFT1155 is
    Initializable,
    UUPSUpgradeable,
    ERC1155SupplyUpgradeable,
    ReentrancyGuardUpgradeable,
    PausableUpgradeable,
    Ownable2StepUpgradeable,
    IPhiNFT1155,
    Claimable,
    CreatorRoyaltiesControl
{

The problem with the above is that inheriting from PausableUpgradeable is not effective in the scope of OZ ERC1155 contracts. As a result, users are able to transfer NFT tokens even when the contract is paused, as the below PoC demonstrates.

Proof of Concept

Drop this test to PhiFactory.t.sol and execute via forge test --match-test Kuprum

function testKuprum_PhiNFT1155PauseNotWorking() public {
    _createArt(ART_ID_URL_STRING);
    uint256 artId = 1;
    bytes32 advanced_data = bytes32("1");
    bytes memory signData =
        abi.encode(expiresIn, participant, referrer, verifier, artId, block.chainid, advanced_data);
    bytes32 msgHash = keccak256(signData);
    bytes32 digest = ECDSA.toEthSignedMessageHash(msgHash);
    (uint8 v, bytes32 r, bytes32 s) = vm.sign(claimSignerPrivateKey, digest);
    if (v != 27) s = s | bytes32(uint256(1) << 255);
    bytes memory signature = abi.encodePacked(r, s);
    bytes memory data =
        abi.encode(1, participant, referrer, verifier, expiresIn, uint256(1), advanced_data, IMAGE_URL, signature);
    bytes memory dataCompressed = LibZip.cdCompress(data);
    uint256 totalMintFee = phiFactory.getArtMintFee(1, 1);

    vm.startPrank(participant, participant);
    phiFactory.claim{ value: totalMintFee }(dataCompressed);

    // referrer payout
    address artAddress = phiFactory.getArtAddress(1);
    assertEq(IERC1155(artAddress).balanceOf(participant, 1), 1, "particpiant erc1155 balance");

    // Everything up to here is from `test_claim_1155_with_ref`

    // Owner pauses the art contract
    vm.startPrank(owner);
    phiFactory.pauseArtContract(artAddress);

    // Users are still able to transfer NFTs despite the contract being paused
    assertEq(IERC1155(artAddress).balanceOf(user1, 1), 0, "user1 doesn't have any tokens");
    vm.startPrank(participant);
    IERC1155(artAddress).safeTransferFrom(participant, user1, 1, 1, hex"00");
    assertEq(IERC1155(artAddress).balanceOf(participant, 1), 0, "particpiant now has 0 tokens");
    assertEq(IERC1155(artAddress).balanceOf(user1, 1), 1, "user1 now has 1 token");
}

Tools Used

Manual review; Foundry

Recommended Mitigation Steps

We recommend to inherit from ERC1155PausableUpgradeable instead of PausableUpgradeable: this enforces the paused state on the NFT transfer functions. Apply the following diff to PhiNFT1155.sol:

diff --git a/src/art/PhiNFT1155.sol b/src/art/PhiNFT1155.sol
index a280d05..258cb5a 100644
--- a/src/art/PhiNFT1155.sol
+++ b/src/art/PhiNFT1155.sol
@@ -8,7 +8,8 @@ import { Claimable } from "../abstract/Claimable.sol";
 import { ERC1155Upgradeable } from "@openzeppelin/contracts-upgradeable/token/ERC1155/ERC1155Upgradeable.sol";
 import { ERC1155SupplyUpgradeable } from
     "@openzeppelin/contracts-upgradeable/token/ERC1155/extensions/ERC1155SupplyUpgradeable.sol";
-import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/PausableUpgradeable.sol";
+import { ERC1155PausableUpgradeable } from 
+    "@openzeppelin/contracts-upgradeable/token/ERC1155/extensions/ERC1155PausableUpgradeable.sol";
 import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/ReentrancyGuardUpgradeable.sol";
 import { Ownable2StepUpgradeable } from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";
 import { SafeTransferLib } from "solady/utils/SafeTransferLib.sol";
@@ -23,12 +24,21 @@ contract PhiNFT1155 is
     UUPSUpgradeable,
     ERC1155SupplyUpgradeable,
     ReentrancyGuardUpgradeable,
-    PausableUpgradeable,
+    ERC1155PausableUpgradeable,
     Ownable2StepUpgradeable,
     IPhiNFT1155,
     Claimable,
     CreatorRoyaltiesControl
 {
+    // The following functions are overrides required by Solidity.
+
+    function _update(address from, address to, uint256[] memory ids, uint256[] memory values)
+        internal
+        override(ERC1155PausableUpgradeable, ERC1155SupplyUpgradeable)
+    {
+        super._update(from, to, ids, values);
+    }
+
     /*//////////////////////////////////////////////////////////////
                                  USING
     //////////////////////////////////////////////////////////////*/

Assessed type

Library

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report

[McCoady]

Why should the transferability of tokens be pausable or where did the sponsor point out that this is the intended behaviour? Many NFT collections implement a pause functionality around their core minting/claiming logic should something go wrong during mint, however it is non-standard to pause the transfer of already minted tokens, as this would effectively rug the collection as it would force all tokens to become untradable.

The whenNotPaused modifier is correctly applied to the functions in control of creating and claiming new art (createArtFromFactory and claimFromFactory), but I'm unclear why the protocol would wish to pause the transfer of already minted tokens?

[kuprumxyz]

Contract pausability is designed in general to be activated in case bugs are discovered in contracts. It is a common practice to pause all contract operations as soon as possible: when a bug in discovered it's not known what the effects of that bug are: it may very well happen that it allows unrestricted transfers, and thus transfers need also be paused to protect users from the possible exploit.

Besides, there is a solid reason why ERC1155PausableUpgradeable exists, which does prevent tokens from being transferable in the paused state, and it's apparently a simple oversight not to inherit from that contract.

[McCoady]

Your points would have more validity if the sponsor had blindly inherited the OZ ERC-1155 transfer functionality, however as they explicitly override both the transfer functions they inherit from ERC1155 (to implement their own soulBounded transfer check) and choose to not include any pause check it's reasonable to assume it's a design choice to leave these functions unpaused rather than an oversight.

In PhiNFT1155.sol:

    function safeTransferFrom(
        address from_,
        address to_,
        uint256 id_,
        uint256 value_,
        bytes memory data_
    )
        public
        override
    {
        if (from_ != address(0) && soulBounded(id_)) revert TokenNotTransferable();
        address sender = _msgSender();
        if (from_ != sender && !isApprovedForAll(from_, sender)) {
            revert ERC1155MissingApprovalForAll(sender, from_);
        }

        _safeTransferFrom(from_, to_, id_, value_, data_);
    }

    function safeBatchTransferFrom(
        address from_,
        address to_,
        uint256[] memory ids_,
        uint256[] memory values_,
        bytes memory data_
    )
        public
        override
    {
        for (uint256 i; i < ids_.length; i++) {
            if (from_ != address(0) && soulBounded(ids_[i])) revert TokenNotTransferable();
        }
        address sender = _msgSender();
        if (from_ != sender && !isApprovedForAll(from_, sender)) {
            revert ERC1155MissingApprovalForAll(sender, from_);
        }
        _safeBatchTransferFrom(from_, to_, ids_, values_, data_);
    }

[kuprumxyz]

Exactly: they had to overwrite the functions only to account for the soulBounded parameter (which is going to be removed btw.). And if the contract inherited from ERC1155PausableUpgradeable, then _safeTransferFrom and _safeBatchTransferFrom would automatically revert when paused. So, again, I believe it's a simple oversight.

Why do you try to impose on the sponsor the design choices that weren't made? That's just an attempt to invalidate a valid finding. @ZaK3939 confirmed the previously selected #61, so unless the sponsor comments to the contrary this issue and its duplicates are valid.

[McCoady]

I'm not trying to impose anything on the sponsor and this is not an attack on you or your finding (you're a talented SR whom I respect so I hope you don't take it this way).

I'm only pointing out when that a warden reads a codebase and some functions are marked whenNotPaused and others are not, the natural assumption to assume this is a design choice rather than an error on the part of the developer (for example updateRoyalties also doesn't have the whenNotPaused modifier).

I don't want to keep going back and forth and will leave it up the judge from here.

[kuprumxyz]

No worries; we all try to do our best:)

Here is one concrete scenario which demonstrates that transfer functions should be pausable:

1. The artist submits createArt transaction to create PhiNFT1155 contract. Their intention is to have soulBounded set to true, i.e. to prohibit transferring tokens;
2. This transaction got frontrunned as explained in #70, and in the config soulBounded is set to false, and the artist is also impersonated.
3. PhiNFT1155 contract gets created, and the changes to the config are unnoticed. Tokens are minted, everything operates as it should.
4. Then the artist notices that the users do transfer tokens despite that they should not be able to. The artist tries to alter the config via updateArtSettings, but they are not able to, because they are not registered as the art creator.
5. The artist notifies the admins, and the PhiNFT1155 contract is paused. But because the transfers are not pausable, users are still able to transfer tokens, despite the artist and the admins intention to prohibit that.

I hope that's enough explanation of why the transfers should be pausable.

I am also leaving it up to the judge from here.

[fatherGoose1]

Given the sponsor's acceptance of the issue via comment thank you so much, it can be concluded that omitting the whenNotPaused function on the transfer functions was indeed an oversight.