Skip to content
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

/market/price insufficent checks resulting in invalid memory address or nil pointer dereference #746

Open
pirinskodxb opened this issue Feb 28, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@pirinskodxb
Copy link

Reproduce:
TDEXD running with a market created with no deposits (perhaps also when funds used up) to either base or quote asset.

An unopen market can still be accessed directly via the base/quote params to /market/price and still trigger the issue although most public clients adhere to the markets within /markets/.

hitting /market/price will result in nil pointer dereference.

/tdex/fun/tdex-daemon/internal/core/application/trade/service.go:92 +0x1d2

baseAssetBalance := balance[mkt.BaseAsset].GetConfirmedBalance()
log.Debug(baseAssetBalance)
quoteAssetBalance := balance[mkt.QuoteAsset].GetConfirmedBalance()
log.Debug(quoteAssetBalance)

@altafan altafan added the bug Something isn't working label Feb 28, 2024
@altafan
Copy link
Contributor

altafan commented Feb 28, 2024

@pirinskodxb Thanks for spotting this out.

I was a bit scared because I thought the daemon was panicking, but once I reproduced, I realized that it's returning a non-formatted error to the client.

This can be easily fixed by checking that the market is open before returning its price.

Screenshot 2024-02-28 at 14 03 53

@pirinskodxb
Copy link
Author

I think the grpc thread does panic but the daemon handles it. I had to mod the UnaryServerInterceptor() to get the backtrace. Perhaps markets shouldn't be openable without sufficent funds.

Users could potentially bypass checks at /markets/ since all they need are asset ID's which are public of course, not sure what potential ramifications that could have but now I will figure out how to make a trade and see what else I discover :)

@altafan
Copy link
Contributor

altafan commented Feb 28, 2024

Indeed. Will create a ticket for this. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants