[Bug] Resource account creation can be blocked

[ahermida]

🐛 Bug

The creation of a resource account can be blocked by somebody calling: aptos_framework::aptos_account::transfer(&me, future_resource_account_addr, 1);

To reproduce

Code snippet to reproduce

    #[test(account = @0x2048, core = @0x1)]
    public entry fun test_resource_account(account: signer, core: signer) {

        // initialize account with some apt
        let (burn_cap, mint_cap) = aptos_framework::aptos_coin::initialize_for_test(&core);
        aptos_framework::aptos_account::create_account(std::signer::address_of(&account));
        std::coin::deposit(std::signer::address_of(&account), std::coin::mint(10000, &mint_cap));

        // create resource address and allocate funds
        let acct_addr = aptos_framework::account::create_resource_address(&std::signer::address_of(&account), b"broken");
        aptos_framework::aptos_account::transfer(&account, acct_addr, 1);

        // attempt to create resource account - WILL FAIL HERE
        aptos_framework::account::create_resource_account(&account, b"broken");

        // cleanup
        std::coin::destroy_burn_cap(burn_cap);
        std::coin::destroy_mint_cap(mint_cap);
    }

Stack trace/error message

┌── test_resource_account ──────
│ ITE: An unknown error was reported. Location: error[E11001]: test failure
│     ┌─ /Users/my_cool_username/.move/https___github_com_aptos-labs_aptos-core_git_devnet/aptos-move/framework/aptos-framework/sources/account.move:150:9
│     │
│ 134 │     fun create_account_unchecked(new_address: address): signer {
│     │         ------------------------ In this function in 0x1::account
│     ·
│ 150 │         move_to(
│     │         ^^^^^^^
│ 
│ 
│ VMError (if there is one): VMError {
│     major_status: RESOURCE_ALREADY_EXISTS,
│     sub_status: None,
│     message: None,
│     exec_state: None,
│     location: Module(
│         ModuleId {
│             address: 0000000000000000000000000000000000000000000000000000000000000001,
│             name: Identifier(
│                 "account",
│             ),
│         },
│     ),
│     indices: [],
│     offsets: [
│         (
│             FunctionDefinitionIndex(4),
│             41,
│         ),
│     ],
│ }
└──────────────────

Test result: FAILED. Total tests: 2; passed: 1; failed: 1
{
  "Error": "Move unit tests failed"
}

Expected Behavior

I would expect the (signer, SignerCapability) to be retrieved with the APT I sent to it and then a struct assigned to the address like struct ResourceAccount has key {} so it couldn't be called again.

System information

Please complete the following information:

• Aptos CLI 0.3.3 / devnet https://github.com/aptos-labs/aptos-core/commit/8f634bcd2e48c4972868d0053d3532f4f760ae19
• MacOS intel 12.5.1 (21G83)

Additional context

I'm thinking about using resource accounts for permissions in my dapp for related data (ex: user + some_authority + b"special_escrow_wallet" = lease_addr)

[chen-robert]

You don't even need to transfer funds, the root cause appears to be

1. aptos_framework::account::create_resource_account is a one time operation
2. aptos_framework::aptos_account gives you an arbitrary account creation primitive

   public entry fun create_account(auth_key: address) {
       let signer = account::create_account(auth_key);
       coin::register<AptosCoin>(&signer);
   }

Perhaps create_resource_account should check if the account exists and then skip create_account_unchecked if it already exists?

[alnoki]

@ahermida I have addressed this per the most recent commit to PR #4173, see create_resource_account_force_creation().

[ahermida]

I was pretty excited about this feature, but if a user cannot usurp control of a resource account that's been front-run, then it breaks designs where one might rely on the determinism of these addresses elsewhere in the application.

@davidiw @alnoki why not just allow the origin signer to claim the account on create_resource_account (just once)?

[alnoki]

but if a user cannot usurp control of a resource account that's been front-run

@ahermida How would you go about determining who is the legitimate user and who is the front-runner? What about an illegitimate usurper?

[ahermida]

I would assume that if create_resource_address(&origin, seed) results in an address, the signer origin would have authority over that address. I'm making a blanket assumption that would be impossible for an attacker to predict a seed that intersects with another existing account. Am I wrong?

[alnoki]

I would assume that if create_resource_address(&origin, seed) results in an address, the signer origin would have authority over that address. I'm making a blanket assumption that would be impossible for an attacker to predict a seed that intersects with another existing account. Am I wrong?

Indeed origin has authority over the resultant resource account address, if they call create_resource_account() with the same parameters.

As for the second part, it depends what you mean by "existing account": if it refers to an origin account, then front-running is as simple as create_resource_account(&account_to_front_run, &seed_to_front_run).

If "existing account" refers to a resource account, see @chen-robert 's snippet above on blocking it even without a seed.

[ahermida]

@alnoki thanks for the quick response

I meant that if the origin is a signer type, then they should have authority over any address generated with a seed and their address (and should be able to usurp that authority if there exists a collision just once).

And by "existing account" I mean another account that is targeted by the user to usurp authority. I'm a bit out of my depth in this realm, but I'd assume that a user cannot find a collision with another aptos account to steal their funds. Maybe they can?

[ahermida]

@alnoki you're right - this isn't something we can do without some rethinking. I forgot why pda's have bumps

[alnoki]

I meant that if the origin is a signer type, then they should have authority over any address generated with a seed and their address (and should be able to usurp that authority if there exists a collision just once).

Now this is an interesting access control proposal, and from first principles it seems logical to grant a given origin the authority to seize any resource account generated using their address as an input. An issue here, though, would then be if a given {origin, seed} collides with another {origin_2, seed_2}:

Since there are theoretically infinite inputs for a seed, which map onto a finite number of resource account addresses, it is thus impossible to prevent a collision, and by extension, assert than any given origin is the rightful owner of any given resource address.

[chen-robert]

@ahermida @alnoki The purpose of bumps for PDAs is to guarantee that the address cannot be signed for. Although I believe in practice, the probability of a collision is astronomically low such that it's a non-issue. In other words, if you could collide seeds it seems possible that you would also be able to collide private keys or sign for an arbitrary address.

I'm not sure if the current implementation of create_resource_account_force_creation is practical security-wise, in particular, because it breaks the one-to-one mapping of seeds to addresses. It also seems to open up a large attack vector in the form of reinitialization attacks with any given set of seeds.

[alnoki]

I'm not sure if the current implementation of create_resource_account_force_creation is practical security-wise, in particular, because it breaks the one-to-one mapping of seeds to addresses. It also seems to open up a large attack vector in the form of reinitialization attacks with any given set of seeds.

You are right about reinit attacks with a given set of seeds, but here it is a kind of game of chicken: by calling create_resource_account_force_creation(), the caller signals to malicious actors that they are willing to pay until they get an account. For a malicious actor to block they have to pay even more to init a bunch of blocking accounts. As for the one-to-one mapping, does this problem also apply to create_resource_account()?

[chen-robert]

As for the one-to-one mapping, does this problem also apply to create_resource_account()?

It should not, because the current behavior is to abort if the account already exists (which I think is good).

[PaulFidika]

The root problem is this:

aptos_framework::aptos_account

   public entry fun create_account(auth_key: address) {
        let signer = account::create_account(auth_key);
        coin::register<AptosCoin>(&signer);
    }

This seems way too permissive; do we really want ANYONE to be able to create an account at ANY address arbitrarily? Why? What is the use case for this?

I would think that if you want to create an account for address-ABC, you should be limited to either (1) having the corresponding private key to that address, meaning you can sign for address-ABC to produce a signer object, or (2) it should be an account-address derived from another signer account. That is, you'd get something like:

public entry fun create_account(pubkey, proof_you_have_private_key)

public entry fun create_derived_account(account: &signer, seed_vector)

create_derived_account would combine your signer address + the seed vector to create a deterministic address. The key is that no one should be able to register the existence of this account aside from you (the signer). I.e., a program could exist at address XYZ, and then just bump a single number each time it needs to create a new resource address.

On Solana, PDA (program derived accounts) have an address that is a deterministic function of the signer's address + the token's mint address that is being stored. This allows Solana programs to know where they should be looking for a person's coin balance, even if it does not exist yet. That is, we can compute addresses statically / offline; no need to check the chain to see if the address exists or not. Furthermore these are all off-curve addresses, meaning they will never have a corresponding private key.

For reference, here is how it looks on Solana:

fun createProgramAddress(seeds: List<ByteArray>, programId: PublicKey): PublicKey {
            val buffer = ByteArrayOutputStream()
            for (seed in seeds) {
                require(seed.size <= 32) { "Max seed length exceeded" }
                try {
                    buffer.write(seed)
                } catch (e: IOException) {
                    throw RuntimeException(e)
                }
            }
            try {
                buffer.write(programId.toByteArray())
                buffer.write("ProgramDerivedAddress".toByteArray())
            } catch (e: IOException) {
                throw RuntimeException(e)
            }
            val hash = Sha256Hash.hash(buffer.toByteArray())
            if (TweetNaclFast.is_on_curve(hash) != 0) {
                throw RuntimeException("Invalid seeds, address must fall off the curve")
            }
            return PublicKey(hash)
        }

        @Throws(Exception::class)
        fun findProgramAddress(
            seeds: List<ByteArray>,
            programId: PublicKey
        ): ProgramDerivedAddress {
            var nonce = 255
            val address: PublicKey
            val seedsWithNonce: MutableList<ByteArray> = ArrayList()
            seedsWithNonce.addAll(seeds)
            while (nonce != 0) {
                address = try {
                    seedsWithNonce.add(byteArrayOf(nonce.toByte()))
                    createProgramAddress(seedsWithNonce, programId)
                } catch (e: Exception) {
                    seedsWithNonce.removeAt(seedsWithNonce.size - 1)
                    nonce--
                    continue
                }
                return ProgramDerivedAddress(address, nonce)
            }
            throw Exception("Unable to find a viable program address nonce")
        }

        @Throws(Exception::class)
        fun associatedTokenAddress(walletAddress: PublicKey, tokenMintAddress: PublicKey) : ProgramDerivedAddress {
            return findProgramAddress(
                listOf(
                    walletAddress.toByteArray(),
                    TokenProgram.PROGRAM_ID.toByteArray(),
                    tokenMintAddress.toByteArray()
                ),
                PublicKey("ATokenGPvbdGVxr1b2hvZbsiqW5xWH25efTNsLJA8knL")
            )
        }

In my opinion, this is the correct way to do it. Lmk if I missed anything.

[davidiw]

Addressed in https://github.com/aptos-labs/aptos-core/pull/4598

The problem with trying to reinforce the create_address api is that this would break every single exchange / dapp out there and even if we did that it would add a lot more friction to users. They already have a 66 character address string. They'd be expected to provide both their 66 character seed and 66 character address.