Skip to content

Commit

Permalink
Allow editing max spend limit (MetaMask#7919)
Browse files Browse the repository at this point in the history
In the case where the initial spend limit for the `approve` function
was set to the maximum amount, editing this value would result in the
new limit being silently ignored. The transaction would be submitted
with the original max spend limit.

This occurred because function to generate the new custom data would
look for the expected spend limit in the existing data, then bail if
it was not found (and in these cases, it was never found).

The reason the value was not found is that it was erroneously being
converted to a `Number`. A JavaScript `Number` is not precise enough to
represent larger spend limits, so it would give the wrong hex value
(after rounding had taken place in the conversion to a floating-point
number).

The data string is now updated without relying upon the original token
value; the new value is inserted after the `spender` argument instead,
as the remainder of the `data` string is guaranteed to be the original
limit. Additionally, the conversion to a `Number` is now omitted so
that the custom spend limit is encoded correctly.

Fixes MetaMask#7915
  • Loading branch information
Gudahtt authored and yqrashawn committed Feb 10, 2020
1 parent 568a563 commit 5b68d03
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 52 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import Modal from '../../modal'
import Identicon from '../../../ui/identicon'
import TextField from '../../../ui/text-field'
import classnames from 'classnames'
import BigNumber from 'bignumber.js'

export default class EditApprovalPermission extends PureComponent {
static propTypes = {
Expand Down Expand Up @@ -60,7 +61,7 @@ export default class EditApprovalPermission extends PureComponent {
<div>{t('balance')}</div>
</div>
<div className="edit-approval-permission__account-info__balance">
{`${tokenBalance} ${tokenSymbol}`}
{`${Number(tokenBalance).toPrecision(9)} ${tokenSymbol}`}
</div>
</div>
<div className="edit-approval-permission__edit-section">
Expand Down Expand Up @@ -93,15 +94,17 @@ export default class EditApprovalPermission extends PureComponent {
'edit-approval-permission__edit-section__option-label--selected': selectedOptionIsUnlimited,
})}
>
{tokenAmount < tokenBalance
? t('proposedApprovalLimit')
: t('unlimited')}
{
(new BigNumber(tokenAmount)).lessThan(new BigNumber(tokenBalance))
? t('proposedApprovalLimit')
: t('unlimited')
}
</div>
<div className="edit-approval-permission__edit-section__option-description">
{t('spendLimitRequestedBy', [origin])}
</div>
<div className="edit-approval-permission__edit-section__option-value">
{`${tokenAmount} ${tokenSymbol}`}
<div className="edit-approval-permission__edit-section__option-value" >
{`${Number(tokenAmount)} ${tokenSymbol}`}
</div>
</div>
</div>
Expand Down Expand Up @@ -139,9 +142,8 @@ export default class EditApprovalPermission extends PureComponent {
<TextField
type="number"
min="0"
placeholder={`${customTokenAmount ||
tokenAmount} ${tokenSymbol}`}
onChange={event => {
placeholder={ `${Number(customTokenAmount || tokenAmount)} ${tokenSymbol}` }
onChange={(event) => {
this.setState({ customSpendLimit: event.target.value })
if (selectedOptionIsUnlimited) {
this.setState({ selectedOptionIsUnlimited: false })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,12 +113,8 @@ export default class ConfirmApproveContent extends Component {
{t('accessAndSpendNotice', [origin])}
</div>
<div className="flex-row">
<div className="confirm-approve-content__label">
{t('amountWithColon')}
</div>
<div className="confirm-approve-content__medium-text">
{`${customTokenAmount || tokenAmount} ${tokenSymbol}`}
</div>
<div className="confirm-approve-content__label">{ t('amountWithColon') }</div>
<div className="confirm-approve-content__medium-text">{ `${Number(customTokenAmount || tokenAmount)} ${tokenSymbol}` }</div>
</div>
<div className="flex-row">
<div className="confirm-approve-content__label">
Expand Down
14 changes: 7 additions & 7 deletions ui/app/pages/confirm-approve/confirm-approve.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export default class ConfirmApprove extends Component {
static propTypes = {
tokenAddress: PropTypes.string,
toAddress: PropTypes.string,
tokenAmount: PropTypes.number,
tokenAmount: PropTypes.string,
tokenSymbol: PropTypes.string,
fiatTransactionTotal: PropTypes.string,
ethTransactionTotal: PropTypes.string,
Expand All @@ -31,7 +31,7 @@ export default class ConfirmApprove extends Component {
}

static defaultProps = {
tokenAmount: 0,
tokenAmount: '0',
}

state = {
Expand Down Expand Up @@ -67,26 +67,26 @@ export default class ConfirmApprove extends Component {
} = this.props
const { customPermissionAmount } = this.state

const tokensText = `${tokenAmount} ${tokenSymbol}`
const tokensText = `${Number(tokenAmount)} ${tokenSymbol}`

let tokenBalance

if (Array.isArray(tokenTrackerBalance)) {
tokenBalance = tokenTrackerBalance.map(balance =>
(balance
? Number(calcTokenAmount(tokenTrackerBalance, decimals)).toPrecision(
9
10
)
: '')
)
} else {
tokenBalance = tokenTrackerBalance
? Number(calcTokenAmount(tokenTrackerBalance, decimals)).toPrecision(9)
? Number(calcTokenAmount(tokenTrackerBalance, decimals)).toPrecision(10)
: ''
}

const customData = customPermissionAmount
? getCustomTxParamsData(data, { customPermissionAmount, tokenAmount, decimals })
? getCustomTxParamsData(data, { customPermissionAmount, decimals })
: null

return (
Expand All @@ -102,7 +102,7 @@ export default class ConfirmApprove extends Component {
this.setState({ customPermissionAmount: newAmount })
}}
customTokenAmount={String(customPermissionAmount)}
tokenAmount={String(tokenAmount)}
tokenAmount={tokenAmount}
origin={origin}
tokenSymbol={tokenSymbol}
tokenBalance={tokenBalance}
Expand Down
3 changes: 1 addition & 2 deletions ui/app/pages/confirm-approve/confirm-approve.container.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ const mapStateToProps = (state, ownProps) => {
const tokenData = getTokenData(data)
const tokenValue = tokenData && getTokenValue(tokenData.params)
const toAddress = tokenData && getTokenToAddress(tokenData.params)
const tokenAmount =
tokenData && calcTokenAmount(tokenValue, decimals).toNumber()
const tokenAmount = tokenData && calcTokenAmount(tokenValue, decimals).toString(10)
const contractExchangeRate = contractExchangeRateSelector(state)

const { origin } = transaction
Expand Down
53 changes: 25 additions & 28 deletions ui/app/pages/confirm-approve/confirm-approve.util.js
Original file line number Diff line number Diff line change
@@ -1,36 +1,33 @@
import { decimalToHex } from '../../helpers/utils/conversions.util'
import { calcTokenValue } from '../../helpers/utils/token-util.js'
import { getTokenData } from '../../helpers/utils/transactions.util'

export function getCustomTxParamsData (
data,
{ customPermissionAmount, tokenAmount, decimals }
) {
if (customPermissionAmount) {
const tokenValue = decimalToHex(calcTokenValue(tokenAmount, decimals))
export function getCustomTxParamsData (data, { customPermissionAmount, decimals }) {
const tokenData = getTokenData(data)

const re = new RegExp('(^.+)' + tokenValue + '$')
const matches = re.exec(data)

if (!matches || !matches[1]) {
return data
}
let dataWithoutCurrentAmount = matches[1]
const customPermissionValue = decimalToHex(
calcTokenValue(Number(customPermissionAmount), decimals)
)
if (!tokenData) {
throw new Error('Invalid data')
} else if (tokenData.name !== 'approve') {
throw new Error(`Invalid data; should be 'approve' method, but instead is '${tokenData.name}'`)
}
let spender = tokenData.params[0].value
if (spender.startsWith('0x')) {
spender = spender.substring(2)
}
const [signature, tokenValue] = data.split(spender)

const differenceInLengths = customPermissionValue.length - tokenValue.length
const zeroModifier = dataWithoutCurrentAmount.length - differenceInLengths
if (differenceInLengths > 0) {
dataWithoutCurrentAmount = dataWithoutCurrentAmount.slice(0, zeroModifier)
} else if (differenceInLengths < 0) {
dataWithoutCurrentAmount = dataWithoutCurrentAmount.padEnd(
zeroModifier,
0
)
}
if (!signature || !tokenValue) {
throw new Error('Invalid data')
} else if (tokenValue.length !== 64) {
throw new Error('Invalid token value; should be exactly 64 hex digits long (u256)')
}

const customTxParamsData = dataWithoutCurrentAmount + customPermissionValue
return customTxParamsData
let customPermissionValue = decimalToHex(calcTokenValue(customPermissionAmount, decimals))
if (customPermissionValue.length > 64) {
throw new Error('Custom value is larger than u256')
}

customPermissionValue = customPermissionValue.padStart(tokenValue.length, '0')
const customTxParamsData = `${signature}${spender}${customPermissionValue}`
return customTxParamsData
}

0 comments on commit 5b68d03

Please sign in to comment.