Category: Coding Mistakes
Incorrect error check in TWAP price
Low Severity
Low Impact
High Likelihood
Description
When getting the price for a coin using the Twap strategy, sla.twapKeeper.GetArithmeticTwapToNow is called to calculate the price:
func (sla SpendLimitAuthenticator) getPriceInQuoteDenom(ctx sdk.Context, coin sdk.Coin) (osmomath.Dec, error) {
switch sla.priceStrategy {
case Twap:
// This is a very bad and inefficient implementation that should be improved
oneWeekAgo := ctx.BlockTime().Add(-time.Hour * 24 * 7)
numPools := sla.poolManagerKeeper.GetNextPoolId(ctx)
for i := uint64(1); i < numPools; i++ {
price, err := sla.twapKeeper.GetArithmeticTwapToNow(ctx, i, coin.Denom, sla.quoteDenom, oneWeekAgo)
if err != nil {
return price, nil
}
}
return osmomath.Dec{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidCoins, "no price found for %s", coin.Denom)
case AbsoluteValue:
return osmomath.NewDec(1), nil
default:
return osmomath.Dec{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid price strategy %s", sla.priceStrategy)
}
}The issue is that the error check is inverted, causing the error to be ignored and an empty price to be returned instead.
Impact
When the price is used, it will cause a panic due to a nil pointer dereference, causing the transaction to fail.
Recommendations
The check should be changed to only return the price if err is nil.
Remediation
This issue has been acknowledged by Osmosis Labs, and a fix was implemented in commit 8808c54c↗.