(HAL-16) MOVETH SIGNATURE REPLAY // CRITICAL

[SA124]

(HAL-16) MOVETH SIGNATURE REPLAY Auditor: Halborn Severity: Critical

Description The transfer_from function is vulnerable to signature replay attacks. It uses the account's sequence number as a nonce, which is insufficient to prevent replay attacks: an attacker can reuse a valid signature to execute multiple unauthorized transfers, potentially draining a user's account.

The function uses account::get_sequence_number(from) as a nonce, which doesn't change after a transfer, allowing the same signature to be reused.

The scenario exploiting the vulnerability could be:

1. Alice signs a transfer approval for Bob to transfer 100 tokens.
2. Bob executes the transfer using Alice's signature.
3. Bob can replay the same signature to transfer another 100 tokens without Alice's consent.

Proof of Concept The following test in MOVETH_tests.move passes, which demonstrates the vulnerability, reusing the permit multiple times:

BVSS AO:A/AC:L/AX:L/R:N/S:U/C:N/A:N/I:N/D:C/Y:N (10.0)

Recommendation It is recommended to implement a real nonce for the signed permit. As a reference, the OpenZeppelin's ERC20Permit implements a nonce tracking each permit utilisation.

[andygolay]

@0xmovses @musitdev do we even need the transfer_from function? It's not used in any of the bridge modules.

[musitdev]

I don't know. Every SC calls are done?

[0xmovses]

Correct @andygolay Moveth::transfer_from is not called anywhere inside atomic_bridge_initiator and atomic_bridge_counterparty, however if we were to remove it. It would no longer be a Fungible Asset. Let's add the nonce or sequence number solution suggested, so that a signature is explicitly tied to a specific nonce or sequence number.

[andygolay]

What makes MOVETH a fungible asset is this code in init_module:

        // Create the stablecoin with primary store support.
        let constructor_ref = &object::create_named_object(resource_account, ASSET_SYMBOL);
        primary_fungible_store::create_primary_store_enabled_fungible_asset(
            constructor_ref,
            option::none(),
            utf8(ASSET_SYMBOL), /* name */
            utf8(ASSET_SYMBOL), /* symbol */
            8, /* decimals */
            utf8(b"http://example.com/favicon.ico"), /* icon */
            utf8(b"http://example.com"), /* project */
        );

in particular the create_primary_store_enabled_fungible_asset step.

If you look at the MOVETH test you can see how a transfer is done:

        // transfer from minter to receiver, check balance
        let minter_store = primary_fungible_store::ensure_primary_store_exists(minter_address, asset);
        let receiver_store = primary_fungible_store::ensure_primary_store_exists(receiver_address, asset);
        dispatchable_fungible_asset::transfer(minter, minter_store, receiver_store, 10);

with, in this example, minter being the "user" signing to approve the transfer of their own assets from their primary fungible store to receiver's.

From what I can tell, transfer_from appears to attempt to mimic how transferFrom is used in ERC-20 / Solidity.

In Move, it would be more typical to use dispatchable_fungible_asset::transfer for user-signed FA transfers.

If there's some special usecase for a spender to transfer tokens from someone else's account, then it might make sense for transfer_from to stay in the contract. But the bridge does not use that function and therefore, the transfer_from function appears to be unnecessary and therefore should be removed imo.