M_3. Transfer return value check & Assembly Check [low ~ medium]

MarkInc3 commented 4 months ago

Configuration

Severity: medium
Confidence: medium

Description

Contracts need to support tokens that do not use the ERC20 token standard

Recommendation

Check the return value in the contract and handle it, or use the SafeTransfer library. Additionally, there are implementations that perform the same functionality but save gas.

Exploit Scenario

Lines of Codes

Code to check

 if (chainData[_l2chainId].nativeL1token == _l1token) {
          //need to approve
    IERC20(_l1token).transferFrom(msg.sender, address(this), _fwAmount);
    IERC20(_l1token).transfer(_to,_fwAmount);
} else if (chainData[_l2chainId].legacyERC20ETH == _l1token) {
    require(msg.value == _fwAmount, "FW: ETH need same amount");
    payable(address(this)).call{value: msg.value};
    (bool sent, ) = payable(_to).call{value: msg.value}("");
    require(sent, "claim fail");
} else {
    //need to approve
    IERC20(_l1token).transferFrom(msg.sender, _to, _fwAmount);
}

Demo

Below is a SafeTransfer with the assembly applied.


function safeTransferFrom(
      ERC20 token,
      address from,
      address to,
      uint256 amount
  ) internal {
      bool success;

      /// @solidity memory-safe-assembly
      assembly {
          // Get a pointer to some free memory.
          let freeMemoryPointer := mload(0x40)

          // Write the abi-encoded calldata into memory, beginning with the function selector.
          mstore(freeMemoryPointer, 0x23b872dd00000000000000000000000000000000000000000000000000000000)
          mstore(add(freeMemoryPointer, 4), and(from, 0xffffffffffffffffffffffffffffffffffffffff)) // Append and mask the "from" argument.
          mstore(add(freeMemoryPointer, 36), and(to, 0xffffffffffffffffffffffffffffffffffffffff)) // Append and mask the "to" argument.
          mstore(add(freeMemoryPointer, 68), amount) // Append the "amount" argument. Masking not required as it's a full 32 byte type.

          success := and(
              // Set success to whether the call reverted, if not we check it either
              // returned exactly 1 (can't just be non-zero data), or had no return data.
              or(and(eq(mload(0), 1), gt(returndatasize(), 31)), iszero(returndatasize())),
              // We use 100 because the length of our calldata totals up like so: 4 + 32 * 3.
              // We use 0 and 32 to copy up to 32 bytes of return data into the scratch space.
              // Counterintuitively, this call must be positioned second to the or() call in the
              // surrounding and() call or else returndatasize() will be zero during the computation.
              call(gas(), token, 0, freeMemoryPointer, 100, 0, 32)
          )
      }

      require(success, "TRANSFER_FROM_FAILED");
  }

zzooppii commented 4 months ago

Because there is a caveat, we applied the safeTransfer format, which is widely used, rather than the safeTransfer format, which has not been tested much. https://github.com/tokamak-network/crossTrade/blob/ApplyFirstAudit/contracts/libraries/SafeERC20.sol