Summary
Division after multiplication is performed in several functions in contracts GMXManager.sol and GMXWithdraw.sol. This can lead to loss of tokens due to precision loss.
Vulnerability Details
In Solidity, the order of operations matters, especially when it comes to division and multiplication. This is because division and multiplication are not commutative operations, meaning the order in which they are performed can lead to different results.
In functions GMXManager::calcBorrow, GMXManager::calcMinMarketSlippageAmt, GMXManager::calcMinTokensSlippageAmt, GMXWithdraw::withdraw there are multiple instances where multiplication is performed after division. This can potentially lead to precision loss, which is the loss of information due to the approximation of real numbers to a finite number of digits.
In GMXManager.sol:
function calcBorrow(
GMXTypes.Store storage self,
uint256 depositValue
) external view returns (uint256, uint256) {
uint256 _positionValue = depositValue * self.leverage / SAFE_MULTIPLIER;
uint256 _borrowValue = _positionValue - depositValue;
uint256 _tokenADecimals = IERC20Metadata(address(self.tokenA)).decimals();
uint256 _tokenBDecimals = IERC20Metadata(address(self.tokenB)).decimals();
uint256 _borrowLongTokenAmt;
uint256 _borrowShortTokenAmt;
if (self.delta == GMXTypes.Delta.Long) {
_borrowShortTokenAmt = _borrowValue * SAFE_MULTIPLIER
/ GMXReader.convertToUsdValue(self, address(self.tokenB), 10**(_tokenBDecimals))
/ (10 ** (18 - _tokenBDecimals));
}
if (self.delta == GMXTypes.Delta.Neutral) {
(uint256 _tokenAWeight,) = GMXReader.tokenWeights(self);
uint256 _longTokenWeightedValue = _tokenAWeight * _positionValue / SAFE_MULTIPLIER;
@> _borrowLongTokenAmt = _longTokenWeightedValue * SAFE_MULTIPLIER
/ GMXReader.convertToUsdValue(self, address(self.tokenA), 10**(_tokenADecimals))
/ (10 ** (18 - _tokenADecimals));
@> _borrowShortTokenAmt = (_borrowValue - _longTokenWeightedValue) * SAFE_MULTIPLIER
/ GMXReader.convertToUsdValue(self, address(self.tokenB), 10**(_tokenBDecimals))
/ (10 ** (18 - _tokenBDecimals));
}
return (_borrowLongTokenAmt, _borrowShortTokenAmt);
}
function calcMinMarketSlippageAmt(
GMXTypes.Store storage self,
uint256 depositValue,
uint256 slippage
) external view returns (uint256) {
uint256 _lpTokenValue = self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
false,
false
);
return depositValue
* SAFE_MULTIPLIER
/ _lpTokenValue
@> * (10000 - slippage) / 10000;
}
function calcMinTokensSlippageAmt(
GMXTypes.Store storage self,
uint256 lpAmt,
address minLongToken,
address minShortToken,
uint256 slippage
) external view returns (uint256, uint256) {
uint256 _withdrawValue = lpAmt
* self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
false,
false
)
/ SAFE_MULTIPLIER;
(uint256 _tokenAWeight, uint256 _tokenBWeight) = GMXReader.tokenWeights(self);
uint256 _minLongTokenAmt = _withdrawValue
* _tokenAWeight / SAFE_MULTIPLIER
@> * SAFE_MULTIPLIER
/ GMXReader.convertToUsdValue(
self,
minLongToken,
10**(IERC20Metadata(minLongToken).decimals())
)
/ (10 ** (18 - IERC20Metadata(minLongToken).decimals()));
uint256 _minShortTokenAmt = _withdrawValue
* _tokenBWeight / SAFE_MULTIPLIER
@> * SAFE_MULTIPLIER
/ GMXReader.convertToUsdValue(
self,
minShortToken,
10**(IERC20Metadata(minShortToken).decimals())
)
/ (10 ** (18 - IERC20Metadata(minShortToken).decimals()));
return (
@> _minLongTokenAmt * (10000 - slippage) / 10000,
@> _minShortTokenAmt * (10000 - slippage) / 10000
);
}
In GMXWithdraw.sol:
function withdraw(
GMXTypes.Store storage self,
GMXTypes.WithdrawParams memory wp
) external {
if (self.tokenA.balanceOf(address(this)) > 0) {
self.tokenA.safeTransfer(self.trove, self.tokenA.balanceOf(address(this)));
}
if (self.tokenB.balanceOf(address(this)) > 0) {
self.tokenB.safeTransfer(self.trove, self.tokenB.balanceOf(address(this)));
}
self.refundee = payable(msg.sender);
GMXTypes.HealthParams memory _hp;
_hp.equityBefore = GMXReader.equityValue(self);
_hp.lpAmtBefore = GMXReader.lpAmt(self);
_hp.debtRatioBefore = GMXReader.debtRatio(self);
_hp.deltaBefore = GMXReader.delta(self);
GMXTypes.WithdrawCache memory _wc;
_wc.user = payable(msg.sender);
_wc.shareRatio = wp.shareAmt
* SAFE_MULTIPLIER
/ IERC20(address(self.vault)).totalSupply();
_wc.lpAmt = _wc.shareRatio
@> * GMXReader.lpAmt(self)
/ SAFE_MULTIPLIER;
_wc.withdrawValue = _wc.lpAmt
@> * self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
false,
false
)
/ SAFE_MULTIPLIER;
.
.
.
uint256 _lpAmtToRemove = _wc.lpAmt
* (self.leverage - SAFE_MULTIPLIER)
/ self.leverage
@> * 10200 / 10000;
_wc.tokensToUser = _wc.lpAmt - _lpAmtToRemove;
_wc.lpAmt = _lpAmtToRemove;
}
.
.
.
}
Impact
The performed multiplication after division in functions GMXManager::calcBorrow, GMXManager::calcMinMarketSlippageAmt, GMXManager::calcMinTokensSlippageAmt, GMXWithdraw::withdraw can lead to loss of tokens for the users.
The precision loss in these operations is due to the fact that division of integers in Solidity results in an integer, not a fractional number. If the result of the division operation is a fractional number, it is rounded down to the nearest whole number. This means that any fractional part of the result is lost, leading to a loss of precision.
Tools Used
Manual Review, VS Code
Recommendations
Rewrite the above mentioned functions in such a way that the division is performed at the end of the calculations.
