hats-finance / Tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad

4 stars 4 forks source link

permitTransferFromERC1155 / permitTransferFromERC20 can be frontrun and griefed #13

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x25bdc5b095d9c0a48bbd14b57946335f137f38214d574b2708bb43a371ee0dc1 Severity: medium

Description: Description\

In PermitC,

we have function permitTransferFromERC1155 / permitTransferFromERC20

 function permitTransferFromERC1155(
        address token,
        uint256 id,
        uint256 nonce,
        uint256 permitAmount,
        uint256 expiration,
        address owner,
        address to,
        uint256 transferAmount,
        bytes calldata signedPermit
    ) external returns (bool isError) {
        _requireNotPaused(PAUSABLE_PERMITTED_TRANSFER_FROM_ERC1155);

        _checkPermitApproval(
            TOKEN_TYPE_ERC1155, token, id, permitAmount, nonce, expiration, owner, transferAmount, signedPermit
        );
        isError = _transferFromERC1155(token, owner, to, id, transferAmount);

        if (isError) {
            _restoreNonce(owner, nonce);
        }
    }

the _checkPermitApproval calls

  function _checkPermitApproval(
        uint256 tokenType,
        address token,
        uint256 id,
        uint256 permitAmount,
        uint256 nonce,
        uint256 expiration,
        address owner,
        uint256 transferAmount,
        bytes calldata signedPermit
    ) internal {
        bytes32 digest = _hashTypedDataV4(
            PermitHash.hashSingleUsePermit(tokenType, token, id, permitAmount, nonce, expiration, _masterNonces[owner])
        );

        _checkPermitData(nonce, expiration, transferAmount, permitAmount, owner, digest, signedPermit);
    }

as we can see, the permitAmount is a part of schema to compose the hash,

but the user can input any transferAmount

consider the case:

  1. alice sign a signature and wants to trigger permitTransferFromERC1155 with 10000 amount of ERC1155 token with token id 1.
  2. bob frontrun the permitTransferFromERC1155, use the same signature, but input permitAmount as 10000 to pass the signature validation, yet set the transferAmount to 1 or even 0.0001 token, the check will pass
  if (transferAmount > permitAmount) {
            revert PermitC__SignatureTransferExceededPermittedAmount();
        }

because 1 or 0.0001 token is below the permitAmount 10000.

  1. the bob transaction executes first, the same signature is consumed, so alice cannot transfer 10000 amount ERC1155 token.

same issue applies to ERC20 permit transaction, and does not apply to ERC721 token transfer because the code enforce that the permitAmount and transferAmount are both one.

Attack Scenario\

See above. all permit ERC1155 token transfer and permit ERC20 token can be constantly griefed.

the recommendation is that enforce transferAmount == permitAmount.

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional)

0xRektora commented 5 months ago

We don't use permitTransferFromERC1155 in the code. PermitC is OOS. Only Pearlmit and PearlmitHash are.

JeffCX commented 5 months ago

https://app.hats.finance/audit-competitions/tapioca-0xe0b920d38a0900af3bab7ff0ca0af554129f54ad/scope

is this the scope?

Screenshot 2024-05-28 at 4 21 50 PM
0xRektora commented 5 months ago

@JeffCX This one is Screenshot 2024-05-28 at 4 23 13 PM

JeffCX commented 5 months ago

wait a minutes,

scope

contracts/pearlmit/Pearlmit.sol contracts/pearlmit/PearlmitHash.sol

and if pearlmit is in scope

https://github.com/Tapioca-DAO/tapioca-periph/blob/1c21decbccd0c4d23b0f9daace6347a9d801a5c1/contracts/pearlmit/Pearlmit.sol#L30

the contract inherit from PermitC

contract Pearlmit is PermitC {

also this permitTransferFromERC1155 / permitTransferFromERC20 is there for anyone to use.

0xRektora commented 5 months ago

That's correct, but we don't use it in the protocol. If someone uses it, it's outside of the protocol.