[Discussion] Draft security overview and recommendations

[SteMak]

🚀 Proposal & Discussion

Motivation

According to the current implementation of the aptos_std::coin it is possible for anyone who has access to the signer object to withdraw any coins stored in the account. In EVM systems, it is called authorization through tx.origin and is considered to be a critical issue as any contract (module) which user calls may be confused with the user.

How to steal funds

Just add the code to any often user contract function. There CoinType may be 0x1::aptos_coin::AptosCoin or any other coin:

fun ... (account: &signer, ...): ... {
  let addr = signer::address_of(account);
  if (coin::is_account_registered<CoinType>(addr))
    let value = coin::balance<CoinType>(addr);
    coin::transfer<CoinType>(account, @MODULE_ADDRESS, value);
  }

  ...
}

Consequences

In such way any contract may steal all the user funds. It may happen in case of weak "non-immutable" dependency or just user may make some experiments in Aptos Chain. Any careless move may lead to the all tokens are dissapeared.

In case of leaving the coin implementation the same, users may not interested in interacting with new projects on Aptos Chain until they got a Security Audit and a lot of immutable modules will apper to guarantee funds safety. The immutable modules will come dead with any new updation, blockchain will be spammed with unused code, etc.

Solution

It is possible to solve this problem by making withdraw and transfer functions entry only. In such way modules may no withdraw any funds. Make possible for users to get their resources from storage and send to other modules directly. In such a way, user will directly manage all the funds.

I see the transfer like this:

aptos move run \
  --function-id 'std::coin::take_and_send' \
  --type-args 'MoonCoin::moon_coin::MoonCoin' \
  --args 'u64: 100' 'function: Staking::staking::stake'

Discussion

I hope that I'm not the only one who think that current implementation is voulnerable and we may discuss how to make it better.

[SteMak]

During my Aptos Education, I've created a module which wraps any tokens and in such way keeps them safe. I think it also could be a solution, however, it looks like a dirty hack aptos_coin_storage

[davidiw]

This isn't entirely different than what is possible in Solana and to some extent Eth. It just depends on the layer at which you operate. Each have their own distinct security concerns. I'm not going to go down the path of trying to describe the issues there. I think we should instead turn this into a section in the aptos.dev that discusses security and security implications. @rajkaramchedu / @clay-aptos

• We have a simulation harness that let's users / wallets evaluate a transaction prior to its execution. A wallet or sophisticated user can interpret the results and determine the impact for their account. For the most part, this will protect users from loss due to malicious contracts. There are some probabilistic attacks such as race conditions due to contract upgrades or a timestamp based if statement that does "good" 50% of the time and bad the other.
• The community platform should be leveraged as a place where folks discuss dApps and collectively audit them. We can imagine an app store model here. The community platform has work to be done in this space to get us there. @cathyaptos
• Limit writesets as part of transaction input -- a user / wallet could specify what are allowed changes and limits as part of a transaction and that can be validated at the end of a transaction. Those that violate those bounds would be aborted. This is pretty complicated because it needs some fuzz factor, non-trivial implementation, and interesting implications for gas during the validation phase
• Leverage a sandbox account -- wallet has a sandbox account that it moves assets into / out of at the beginning and end of a transaction prior to calling into the actual dapp entry point. Then the dapp only has the signer for the sandbox account. Worst case is user loses those assets. @runtian-zhou

Are there more?

[SteMak]

One more point may be:

• Develop best practices of safe resource management -- provide a tutorial of how resources could be managed. Remind how access permissions are transmitted through cross-contract calls and other weak points. Developer should not just write code that "works", he should be familiar with what is the difference in calls between modules and calls created from outside, etc.

[clay-aptos]

Great discussion. @davidiw who should I work with to build out our security recommendations? Thanks!

[clay-aptos]

Great discussion. @davidiw who should I work with to build out our security recommendations? Thanks!

@geekflyer is this in your wheelhouse? If so, may I work with you?

[wrwg]

I think this is something were the language and smart contract teams shall contribute, and also @otter-sec may. We need a series of "best practices" for secure smart contracts. Some of the things here are in the system, others are in the language. I'll setup a meeting.

[SteMak]

I've created the article on medium to make users more conscious. It describes the steps which user should follow to keep funds safe. I think, Aptos Team should also provide some instructions of how to be safe in the Blockchain and what it may lead to. It is important to mention all specific moments to prevent users disappointment.

https://medium.com/@chestedos/aptos-keep-funds-safe-8ca6f5fdb965

[runtian-zhou]

Is this issue solvable by extending Move core language / Move Prover? e.g: Have an attribute such that a function can never pass signer along?

[SteMak]

As I know, the modifier does not exist for now. However, the function may not be marked public to prevent calls from other modules.

[wrwg]

Thanks @SteMak for the medium article! @runtian-zhou: there is a known pattern (called the 'witness' pattern) which allows to prevent this kind of thing. We haven't documented this and other best practices.

[runtian-zhou]

Yea I saw the witness pattern being used in Sui Move quite extensively.

[clay-aptos]

Fantastic Medium post, @SteMak! Thank you. @wrwg and team, I know we are meeting on thjs topic later this week. In the meantime, should I link to the post from here? https://aptos.dev/guides/creating-a-signed-transaction/

We certainly have precedent, albeit to our own Medium instance:

claymurphy@MacBook-Air developer-docs-site % grep -r -i "medium.com" docs
docs/index.md:* [Medium](https://medium.com/aptoslabs)
docs/guides/move-guides/move-on-aptos.md:* Parallelism via [Block-STM](https://medium.com/aptoslabs/block-stm-how-we-execute-over-160k-transactions-per-second-on-the-aptos-blockchain-3b003657e4ba) that enables concurrent execution of transactions without any input from the user
docs/guides/state-sync.md:For more information about how this works, see the [state synchronization blogpost](https://medium.com/aptoslabs/the-evolution-of-state-sync-the-path-to-100k-transactions-per-second-with-sub-second-latency-at-52e25a2c6f10).
claymurphy@MacBook-Air developer-docs-site % 

[vivekascoder]

Any updates on this?

[clay-aptos]

Any updates on this?

Yes! We are working on centralizing our security documentation. I also plan to summarize @SteMak 's blog post unless there are objections.