New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eosio.msig can undo account transfers. #53

Closed
StarryJapanNight opened this Issue Aug 23, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@StarryJapanNight
Copy link

StarryJapanNight commented Aug 23, 2018

Problem

There are currently two competing EOS account exchanges live:
https://www.eosnameswaps.com
https://eoscontract.io/exchangename

As documented here,https://medium.com/@motchamotcha/eosio-multisig-contract-vulnerability-b5289d18223f, both dApps are vunerable to unexecuted eosio.msig proposal/approvals that restore an accounts permission after it has been transferred to a new user.

It is currently possible to check the eosio.msig tables for proposal/approvals by the account in question. It is not possible to check whether a proposal that affects the permission of an account has been proposed by a different account. There is therefore no guaranteed in-contract method for transferring accounts. Other applications, such as account lending and ram shorting, can also not be developed.

Possible Solution

Modify eosio.msig to emplace the account name that is being affected by an msig action into a table that can be searched.

Note: There is similar vunerability with deferred transactions.

@StarryJapanNight

This comment has been minimized.

Copy link

StarryJapanNight commented Aug 23, 2018

A simpler solution may be to enforce a rule that msig updateauth transactions can only be proposed by the account being affected by the permission change.

@StarryJapanNight

This comment has been minimized.

Copy link

StarryJapanNight commented Aug 23, 2018

Or perhaps an unapproveall action. There is currently no way to know what msig transactions are approved, but not executed, by an account. The assumption is the account owner approved these; that may or may not be the case.

@moskvanaft

This comment has been minimized.

Copy link
Contributor

moskvanaft commented Aug 24, 2018

@StarryJapanNight ,
from https://medium.com/@motchamotcha/eosio-multisig-contract-vulnerability-b5289d18223f

However, if user2 now executes the multisig proposal

Is there any way to trick user2 into executing the proposal?
If no, I would say that there is no vulnerability here, since people normally don't execute random multisig proposals without reviewing them.
Right now I am not saying that there is nothing to improve here. Just want to make sure that my understanding of severity of this issue is correct.

@moskvanaft

This comment has been minimized.

Copy link
Contributor

moskvanaft commented Aug 24, 2018

There is similar vunerability with deferred transactions.

What do you mean?

@StarryJapanNight

This comment has been minimized.

Copy link

StarryJapanNight commented Aug 24, 2018

User2 and user1 are both controlled by the same individual. By using the user2 account to execute the msig proposal, it will not be listed under user1's scope. Once the user1 account is transfered to a new owner (say user3), there is no way they would know about the proposals made by user2. Yet they have the ability to change user1's permissions at any time.

Deferred actions can be used to set a permissions change that doesn't execute for months later. Again, only by searching blocks can a new owner know this exists.

There needs to be in-contract methods for detecting live msig/deferred transactions.

@StarryJapanNight

This comment has been minimized.

Copy link

StarryJapanNight commented Aug 29, 2018

Can we make a decision on whether this needs attention or not?

@moskvanaft

This comment has been minimized.

Copy link
Contributor

moskvanaft commented Aug 29, 2018

Although selling accounts was not a feature that we originally intended to support, we are planing a fix in multisig contract for this issue.

@moskvanaft

This comment has been minimized.

Copy link
Contributor

moskvanaft commented Aug 31, 2018

@StarryJapanNight
Now multisig contract supports action "invalidate" which invalidates all previous approvals. So, you will need to call it when you transfer ownership of an account (calling it it in the same transaction is the safest).
The fix is available in develop branch. So, it should be included in the next release. Than it will be up to block producers to upgrade multisig contract on mainnet.

@moskvanaft moskvanaft closed this Aug 31, 2018

@StarryJapanNight

This comment has been minimized.

Copy link

StarryJapanNight commented Aug 31, 2018

Excellent work. This is simple for me to implement. Two questions:

  • Will this invalidate all historical msig proposals related to an account? Even before the code change?

  • Is it possible to make a similar change for deferred actions?

@arhag

This comment has been minimized.

Copy link
Contributor

arhag commented Aug 31, 2018

@StarryJapanNight:

Will this invalidate all historical msig proposals related to an account? Even before the code change?

Yes.

Is it possible to make a similar change for deferred actions?

No, for two reasons.

First, it would be a consensus-breaking change which requires a far more sophisticated protocol upgrade (and all the governance/ratification delays associated with that). That by itself is not necessarily a problem and we do intend to have a protocol upgrade release sometime in the near future.

But the second problem is that even with a protocol upgrade, there is a more fundamental problem with the idea of invalidating all deferred transactions that may updateauth of the account in question when the deferred transaction retires.

Currently a contract is the only one with the authorization to cancel a deferred transaction that it generated. It would break the expectations of contracts to allow some outside process to cancel those transactions just because one of the effected users did not want an updateauth changing its permissions to be in limbo in some other contract's generated transaction.

If those deferred transactions cannot be invalidated in general, then at least it would be nice to know all the pending deferred transactions that may update the permissions of the to-be-sold account. There are for sure improvements that we still need to add to allow clients (not smart contracts) to get a better view of the in-flight deferred transactions that may be of interest. These eventual enhancements may help with the task of selling accounts. But the task of safely and fairly selling an EOSIO account is actually very tricky with lots of gotchas and corner cases to look out for.

It seems whatever the safe way to sell EOSIO accounts actually is, it involves temporarily locking up the to-be-sold account in an escrow contract in a restricted mode while auditors (whether manually done by humans or automatically done by code) verify that the blockchain is in a state that makes it safe to complete the trade.

A possible approach to selling EOSIO accounts with various potential pitfalls

The to-be-sold account could be held in an escrow contract in a state in which its permissions structure could not be used to generate additional problematic deferred transactions. Then when all already existing problematic deferred transactions either retired, expired, or were cancelled, it would finally be safe to finish off the transfer and officially transfer ownership from the escrow contract to the new user (by updating the owner authority) and simultaneously move the funds from the escrow contract to the seller.

Unfortunately this requires an oracle to tell the escrow contract when it is safe, because we will not be adding intrinsics to allow contracts the check the status of pending deferred transactions (not that such a feature would be sufficient to solve this complicated issue anyway). That oracle could be a third-party that the account buyer and seller trust, or could just be the account buyer since if the oracle mistakenly approved the trade too soon it would only be to the detriment of the buyer. A deadline should likely be set (especially if the oracle is the buyer) by which time if the oracle has not yet approved the trade, the trade is voided and the properties are returned back to their rightful owners (which in the case of the account likely involves restoring the original backed up owner authority of the account).

For this to work though, the account must be in a state where no further problematic transactions are possible. Otherwise, such a problematic deferred transaction may be generated from a transaction that is executed just prior to the transaction that finalizes the trade. At a minimum this means that the owner authority should be locked to just the escrow contract while it is in escrow, but further restrictions are likely necessary because the seller could still do some damage to the account while it is in escrow (for example transfer away all liquid funds) which can reduce the market value of the account from what the buyer originally agreed to pay. More subtle manipulations or vandalizations are also possible and are only limited by the creativity of contract developers that create new property rights for accounts through innovative smart contracts.

For example, what if the account is DApp account that is locked into a bond to incentivize good behavior? Perhaps the rules of that bond contract allow a "good samaritan" that provides proof of cryptographically provable bad behavior by the DApp account to be rewarded with 20% of the bond amount, while the remaining 80% is burned to ensure the bad actor is punished. Then let's pretend the current DApp owner wants to sell their account with all its property rights and its obligations to some other entity because they are tired of their business and just want to cash out and retire. The bond contract may put significant time restrictions on when they can liquidate those funds and move that off the account. Furthermore, the buyer may not want that since it may disrupt functioning of the DApp which is why they are buying it. So the buyer is willing to take on the responsibilities of that bond contract and has already factored that into the market price of the account. However, while the account is in escrow and just before the buyer approves and therefore activates the final trade, perhaps the seller is able to violate the rules of the bond contract and present their own cryptographic evidence of misdeeds to the contract to collect 20% of their own bond. While a risky move, it could be perfectly rational since if they pull it off right they would end up with the funds from the account sale and an extra 20% of their sizable bond. And as a result, the buyer gets hurt because the account they buy ends up forfeiting the entire bond which was not what they paid for.

Conclusion

As you can see properly implementing an account trading system that closes all of the loopholes is very complicated. I don't see how it can be done safely (in general) in one atomic trade. An escrow contract seems necessary. The exact details of how to set up the restrictions for the account while it is in escrow, how the escrow rules work, and especially what the escrow oracle needs to validate prior to approving the trade need to all be figured out. These processes will likely need to continually be updated and re-evaluated as additional property rights are added through new innovative contracts that the to-be-sold account may have interacted with. So for example, an escrow contract that locks the ability for the account to update the owner authority, transfer tokens, or delegate bandwidth while waiting in escrow so the buyer can ensure there are no problematic in-flight deferred transactions may be sufficient for trading simple accounts that haven't done much more than move some tokens around. But if that account is then involved in a bond contract as described in the example above (and that of course affects the market value of the account that buyers perceive), then that simple escrow contract may no longer be safe for that particular account trade.

It is valuable to first start simple to just get something useful out there. I can see how the account trading service can be incredibly useful to those who just want to buy an account with no liquid tokens, very little staked tokens, and with the minimum amount of RAM both used and available, just so they can buy an account with a desirable name. But even this relatively simple case still requires a fairly sophisticated escrow system to do safely, otherwise it can lead to the serious problems for the buyers as already been discussed in this issue with the eosio.msig loophole (which now has, or rather soon will have, a programmatic solution available via the eosio.msig::invalidate action) and the still-remaining deferred transaction loophole (which unfortunately does not have as clean of a solution).

@StarryJapanNight

This comment has been minimized.

Copy link

StarryJapanNight commented Sep 1, 2018

Thank you for your detailed response. My current solution is once the account ownership has been transferred to the contract, to manually screen it for possible complications before allowing it to be sold. This is effectively the escrow you talk about. While this solution is most definitely not decentralized, nor programmatic, I believe it can resolve most of the potential issues with minimal burden to the developer. Other developers may not like this solution, but for the specific case of account exchanges, I don't currently see any alternative.

One point: If there were a mechanism for detecting active deferred transactions in-contract, it would simplify things for my case, and might be useful for other contract types too.

I appreciate the efforts that have gone into thinking about this problem. The msig code update potentially helps resolve issues for other types of contracts, and is a valued addition in itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment