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

fix: regression of smart contract sell address checks on validateTradeQuote #6081

Merged
merged 2 commits into from
Jan 25, 2024

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Jan 25, 2024

Description

Spotted in https://github.com/shapeshift/web/pull/5971/files#diff-b6f4d6672824399e71740531e3b09efe3ad3fabeb3ad6c4ce0763969f9d39ab7R210

see:

const _isSmartContractAddress = await isSmartContractAddress(quote.receiveAddress)

vs. previous implementation:

const userAddress = useMemo(() => {
if (!sellAssetAccountId) return ''
return fromAccountId(sellAssetAccountId).account
}, [sellAssetAccountId])
const { data: _isSmartContractAddress } = useIsSmartContractAddress(userAddress)

This means that we effectively do not validate on the sell side anymore, but on the receive side instead, which is an extremely high-risk regression.
This however is mitigated by the fact we have a similar and correct check in-place at input step:

const userAddress = useMemo(() => {
if (!sellAssetAccountId) return ''
return fromAccountId(sellAssetAccountId).account
}, [sellAssetAccountId])
const { data: _isSmartContractAddress, isLoading: isAddressByteCodeLoading } =
useIsSmartContractAddress(userAddress)

This PR reverts to the previous behavior.

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes N/A, spotted while working on another PR.

Risk

High Risk PRs Require 2 approvals

  • Low to none as this reverts to the previous behavior. high risk tag becaue this fixes a high-risk regression.

Testing

  • With an actual smart-contract wallet (e.g Gnosis Safe through WC) ensure you are blocked from trading on THOR
  • With Frame-injected smart-contract wallet address, ensure you are blocked from trading on THOR

Engineering

  • ☝🏽

Operations

  • ☝🏽

Screenshots (if applicable)

Tested with frame-injected user smart-contract wallet address

  • Develop

Note isSmartContractAddress returns false, since we're passing the receive DOGE address instead of the smart-contract wallet on the sell side

Screenshot 2024-01-25 at 11 14 35 image
  • This diff
image image

Copy link
Contributor Author

gomesalexandre commented Jan 25, 2024

@gomesalexandre gomesalexandre marked this pull request as ready for review January 25, 2024 10:27
@gomesalexandre gomesalexandre requested a review from a team as a code owner January 25, 2024 10:27
@gomesalexandre gomesalexandre force-pushed the fix_smart_contract_validateQuote_regression branch from 46dc232 to 5bc463c Compare January 25, 2024 12:34
@gomesalexandre gomesalexandre changed the title fix: smart contract sell address checks on validateTradeQuote regression fix: regression of smart contract sell address checks on validateTradeQuote Jan 25, 2024
@gomesalexandre gomesalexandre changed the base branch from develop to release January 25, 2024 13:36
@gomesalexandre gomesalexandre force-pushed the fix_smart_contract_validateQuote_regression branch from acd8ee5 to 46a7e79 Compare January 25, 2024 13:37
@gomesalexandre gomesalexandre changed the base branch from release to develop January 25, 2024 13:37
@gomesalexandre gomesalexandre changed the base branch from develop to release January 25, 2024 13:37
@gomesalexandre gomesalexandre enabled auto-merge (squash) January 25, 2024 17:48
@gomesalexandre gomesalexandre merged commit 5777c92 into release Jan 25, 2024
4 checks passed
@gomesalexandre gomesalexandre deleted the fix_smart_contract_validateQuote_regression branch January 25, 2024 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants