safeTransferFrom allows token theft if destination contract is malicious.

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/tokens/ERC1155Minimal.sol#L90-L118

Vulnerability details

Impact

The safeTransferFrom function trusts that the destination to address will not steal tokens if it is a contract. This could enable an attacker to deploy a malicious contract to trap tokens transferred to it.

An attacker could drain victim token balances by having victims transfer tokens to a malicious contract that does not adhere to the ERC1155 token standard. This could lead to theft of funds.

Scenario

1. Attacker deploys TokenTrap contract

2. Victim calls safeTransferFrom to transfer tokens to the trap:

safeTransferFrom(victim, tokenTrap, tokenId, amount); 

3. TokenTrap implementing onERC1155Received extracts tokens then does not return the correct magic value, keeping funds.

While safeTransferFrom checks return data if to is a contract, it assumes the contract will not steal tokens if the correct return data is provided initially. There is no verification that to actually credited tokens.

Proof of Concept

Look at safeTransferFrom in the ERC1155#safeTransferFrom

function safeTransferFrom(
    address from,
    address to,
    uint256 id,
    uint256 amount,
    bytes calldata data
) public {
    if (!(msg.sender == from || isApprovedForAll[from][msg.sender])) revert NotAuthorized();

    balanceOf[from][id] -= amount;

    // balance will never overflow
    unchecked {
        balanceOf[to][id] += amount;
    }

    afterTokenTransfer(from, to, id, amount);

    emit TransferSingle(msg.sender, from, to, id, amount);

    if (to.code.length != 0) {
        if (
            ERC1155Holder(to).onERC1155Received(msg.sender, from, id, amount, data) !=
            ERC1155Holder.onERC1155Received.selector
        ) {
            revert UnsafeRecipient();
        }
    }
}

The key checks here are:

1. Validate msg.sender is either the from address or an approved operator for from. This prevents unauthorized transfers.

2. Update from and to balances to reflect the transfer amount

The balance update logic itself looks correct - balances cannot be directly manipulated outside of the transfer amount.

However, there is still a risk of theft around trust in the to address:

If to is a malicious contract, it can trap/steal tokens sent to it via safeTransferFrom:

1. Attacker deploys malicious TokenTrap contract
2. Victim calls safeTransferFrom to transfer tokens to trap address
3. TokenTrap.onERC1155Received steals tokens and does not follow standard

While the current proxyContractCheck mitigates this, it's not foolproof if a contract returns the correct bytes4 magic value while stealing tokens behind the scenes later.

To fully protect against this theft vector, an additional pullPayment() style callback from to should be required after safeTransferFrom succeeds but before balances are updated.

Tools Used

Vs Code

Recommended Mitigation Steps

Introduce a pullPayment callback where to must confirm crediting tokens before balances change.

function safeTransferFrom(
  // other params

  if (to.isContract()) {
    // Proxy check

    to.callback(); // pull payment  
  }

  // Update balances
}

This would prevent theft by verifying balances were correctly updated.

Assessed type

Token-Transfer

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid