mysteriumnetwork / payments Goto Github PK

In order to be able to fix them in the future

At the moment we're seeing quite a few issues with transactions being sent but not reflected in the network.

Since this issue seems to be global, need a global way of tracking transactions for both accountant and transactor.

Requirements:

On sending a transaction, the state of the transaction should be checked. If after a period of time the transaction is still not reflected in the network - retry the transaction with the same nonce.

Missing tests:

Sentinel

• Has 0 tests

Observer

• Need more tests for report generation and tracking

Pilvytis / Topperupper

• Redis isn't really tested
• Most DB methods are not tested directly, some are testing in a general workflow of certain actions.
• I assume sched_offchain_test.go could be improved after recent offchain changes.

Hermes

• Could cover redis structure in e2e tests to make sure we don't nuke everything by accident
• Could test for possible race conditions with redis transactions and make sure it doesn't ruin everything

Transactor

• Does not have swapping tests, would be hard to do, but would also be nice. Maybe could just mock some parts of the client to make it easier and instead of e2e test do a unit one.
• Most DB methods aren't covered directly in tests only the workflows

Payments

• package transaction could use a few more tests.
• package gas could use a few more tests.
• package units has no tests.
• package registration has no tests.
• package exchange is very heavily used in multiple places and has little to no tests. Having places like currency.go or api.go tested so that they're most or less set in stone, would be nice.
• package client is missing some tests, for example for addresses.go
• we should go over package crypto and check if anything is missing, a lot of is covered but it has also not received any tests for a long time now.

• move gas interface implementations in separate folders like gas/etherscan or gas/matic
• move registration into the crypto folder (maybe rename crypto to something more accurate?)

Produce a simple service with an attached redis storage which would be able to cache gas prices for some chains and expose them through an API. Reasoning for this:

• We have redundant code in services which keeps pinging multiple gas stations from time to time
• We have to manage 6+ API keys making sure we dont duplicate them in services and risk blocking ourselves
• All services really do the same stuff with the gas station, so why not have a unified API

Services should still be able to send transaction if our gas station is out.

Bonus: The API should have a rate limit if no key (auth) is provided

Add metrics for the fees and transactions delivered. Would be nice to see in a chart how much txs each service is doing and how much it is paying for it

At the moment IsPromiseValid and RecoverSigner are using func ReformatSignatureVForRecovery(signature []byte) error which is mutating promise object signature.

The exchange message has a weird method name, ValidateExchangeMessage. Should be something along the lines of IsMessageValid, IsMessageOK or what not.

After a certain period of time, lets say 30 days, those entries are just simply useless. If they are delivered and are old, they should be removed.

https://docs.polygonscan.com/api-endpoints/gas-tracker

This would decrease risk of getting cheques (promises) which are not covered by any funds because sender did withdrawal faster that receiver did claim his cheque.

It would be nice to have tests for some parts, especially depot and courier at least,

It became clear that our company is going to need some user authentication in order to provide services to users. Namely, it can be useful for following applications:

• VPN service (centralized, subscription-based).
• Myst Nodes (instead of internal user DB, for consolidation).
• Universe (right now it has only one static password).
• Probably for superproxy webscraping service (if we decide to make it a public self-service dashboard with automated access, like packetstream.io).

All it needs to do is to authenticate user using one of configured methods (e.g. plain password) and return JWT token (similar to OAuth 2.0). Services which need to authenticate user will just validate access token and allow access granted for this user.

Note that one user may have multiple confirured authentication methods like password, social account login and so on. For starters lets make only password option, but keep that in mind designing data schema.

Also note that users may belong to different namespaces: i.e. registation database (realm) for webscraping may be different than the one for the VPN service. These accounts are completely different for these services.

Also we will need usual stuff like password resets etc.

It is also possible to consider off-the-shelf options like IdentityServer, but I doubt these are customizable enough, so we'd probably start with own simple thing.

Other services with their own custom observer API implementation should drop it and use the generic observer provided by payments package.

To potentially increase our throughput and make the queue easier to use, we could queue transactions in to separate queues per chain.

At the moment service consumer issued promises are subject to possible replay attacks:

1. Smart contract is deployed on testnet, service consumer issues valid promise which is clearable on smart contract
2. Smart contract is deplyed on mainnet, any promise issued by service consumer is valid on mainnet if service consumer reuses it's identity on mainnnet.
   Solution:
   Add configurable magic constatnt which can be read freely and added to promise hash before signing - that way only smart contract with same magic constant can validate/clear issued promise. Of course constants must be different on different blockchains (i.e "producetion" on mainnet and "test' on "testnet". For better transparancey - this constant can be set on contract deployment only.

There are places in payments where we use context with no timeout resulting in forever hanging BC calls or client creations.

We should have a single thread which we can re-use in all our code. It should be able to to submit transactions in order and return they data to the caller.

This would eliminate the need for custom nonce trackers and rude nonce issues, which are hard to keep up with sometimes.

Include logging in relevant places to try and pin point the issues

Currently something similar exists in node but it is not even close to good enough. We would like to define a custom type which would unify our representation of an address.

Most services now call strings.ToLower(Address.Hex()) when needing to compare these addreses in DB funcs or return them from public facing APIs. Examples include but are not limited:

• https://github.com/mysteriumnetwork/pilvytis/blob/master/storage/dbpil/query_offchain.go#L102
• https://github.com/mysteriumnetwork/pilvytis/blob/master/server/handler/offchain.go#L186
• https://github.com/mysteriumnetwork/pilvytis/blob/master/server/handler/offchain.go#L186

What we want:

• If marshaled such an address should always be lower case
• When using it in comparisons in DB funcs it should be lower case
• When inserting it to DB it should be lower case
• It should still be easy to use and compatible with common package of eth library we use

We need to change the data types for sequenceID and amount in the promise to uint64
https://github.com/mysteriumnetwork/payments/blob/master/promises/promise.go#L31

Hello,
I am very new to this codebase, so I could be easily mistaken, but I believe that the withdraw(...) function in IdentityBalances.sol is vulnerable to a replay attack. And this vulnerability allows stealing other people's tokens.

Pre-conditions:

• A user tops-up his balance
• The same legitimate user calls withdraw(...) with some valid signature parameters.

The attack:
A malicious user can now call the withdraw(...) with the same amount, v, r and s parameters (since they are publicly visible on the block-chain) and transfer the remaining tokens to himself.
The malicious user could call the withdraw(...) function multiple times, as long as the amount is still less than the remaining balances[identity].
The malicious user could call the withdraw(...) function again any time later if the balance is topped-up.

Proposed fix:
Introduce an uint256 nonce for each identity:

mapping(address => uint256) public nonces;

Make the nonce part of the signed hash:

function withdraw(uint256 amount, uint256 nonce, uint8 v, bytes32 r, bytes32 s) public returns (bool) {
      bytes memory p = abi.encodePacked(WITHDRAW_PREFIX, amount, nonce);

Validate that the identity's nonce has not already been used:

  require(nonce > nonces[identity]);

Update the nonce to the latest value:

nonces[identity]  = nonce

Important - the above does not help if the withdraw(...) functions is called with a valid signature, but it reverts due to an insufficient balance. The valid signature would be still publicly visible, but the nonces[identity] would not be updated. Once the balance is topped-up, this failed transaction could be replayed (until the legitimate user does a new withdraw(...) transaction, which would update the nonce). There could be a few possible fixes:

• also increment nonces[identity] on every balance modification (in both, topUp(...) and internalTransfer() functions). This fix introduces some headaches in the future, since any newly added code which would increase the balance of an identity, should also increment the nonce. Maybe it would be better to encapsulate identity balance increase and nonce modification in a single internal function.

• Add an external function to invalide the nonce. Rely on external off-chain code to always call this function in case the withdraw(...) fails.

• Change the code to never revert the withdraw(...) transaction but update the nonce and return false if anything goes wrong.

Currently, transactor uses a system where transactions are stored in the database until they succeed or spend a predefined time in the pending state. Once the time is exceeded, the transactions are then retried with a bigger gas price.

This logic is useful for most transactions and should be extracted to the payment package. We could then use the payment package in both hermes and transactor to use the same logic for both.

The logic should abstract away a transaction, to be able to store and retry any transaction independent of the transaction type itself.

Incrementor tests still sometimes race

Currently, we're implementing a sequence ID counter on the provider side. While coding this, an idea popped up.

What happens if we lose the DB, it gets corrupted or the hard drive explodes? In that case we won't be able to know which sequence ID we're currently working with for the given consumer.

So, to restore the sequence ID we need to expose a function on the IdentityPromises contract, to allow to lookup the last cleared sequenceID for a given receiver.

Once that is implemented, the provider can restore the state in case of a disaster. A lookup should not be expensive, so the provider can lookup which seqID it last used for the promise issuer if he does not know anything about the consumer. Query it once, store it in the database and resume normal book keeping on the local DB.

Migration rules:

• If node had "whitelist" flag on then it should only get "Data transfer" and "Scraping" services enabled
• The rest of nodes should get all 3 services enabled by default (including new users)

Whitelist option should be removed from node code with this task.

All tasks in this epic sorted in priority:

1. mysteriumnetwork/node#5450
2. https://github.com/mysteriumnetwork/infra/issues/1327
3. #234
4. mysteriumnetwork/node#5451

Need to have go bindings for the following:

1. Exit on RootChainManager on L1.
2. Withdraw on ERC20 on Matic(L2).
3. DepositFor on RootChainManager on L1.
4. Matic exits

We currently only cache hermes caller: https://github.com/mysteriumnetwork/payments/blob/master/client/hermes_caller.go

I see no reason to stop there, we should cache all callers and transactor for improved CPU usage in services in expense of a minuscule RAM usage increase. Make sure no race conditions are created with this, probably a mutex should be used.

Born from this: #213

This repo is a building block for many other projects so I think we should have proper docs for it. It will also be useful to have a mental model of it and how it works. I can do it so it'll be useful for the new guy + for me to understand it better.

Documentation: https://docs.metamask.io/guide/registering-your-token.html#code-free-example

This should allow user to click link in our documentation and get prompt in Metamask to add MYST.

Go ethereum uses the standart library JSON package which is slow and inefficient. It does a lot of marshaling and unmarshaling. It would be great to do it more efficiently so we can use less memory/cpu.

• Fork go-ethereum
• Replace the JSON encoding library with "github.com/json-iterator/go"
• Replace that library in other repos:
  • Payments
  • Hermes
  • Observer
  • Transactor

Right now courier is not very modular and clunky to use. It would be nice if we could load it dynamically instead of relying on switch statements.

There seems to be a problem in mainnet where if we give too little gas the transaction takes a while to appear on the blockchain causing us to assume it has failed with an error not found

If current gas price is 30 and we give a low 20 or less even a timer of check every 2 minutes can still result in not found and instant fail for a transaction even though it still exists.

The whole workflow with not found errors should be smarter.