6.1 Donating ovl tokens can cause buildSingle operations to fail
The buildSingle operations allow users to unwind a position and build a new one in the same market using the withdrawn funds as part of the collateral for the new position.
function _buildSingleLogic(...) internal returns (...) {
require(_params.leverage >= ONE, "Shiva:lev<min");
// @audit - Unwind position
_onUnwindPosition(...);
// @audit - Compute collateral for the new positions using all the `ovl` tokens in the contract
uint256 totalCollateral = _params.collateral + ovlToken.balanceOf(address(this));
uint256 tradingFee = _getTradingFee(_params.ovlMarket, totalCollateral, _params.leverage);
bool isLong =
Utils.getPositionSide(_params.ovlMarket, _params.previousPositionId, address(this));
// @audit - Collect remaining tokens from user
// transfer from OVL from user to this contract
ovlToken.transferFrom(_owner, address(this), _params.collateral + tradingFee);
// Approve the ovlMarket contract to spend OVL
_approveMarket(_params.ovlMarket);
// @audit - Build new position
positionId = _onBuildPosition(...);
}
After unwinding the position, all ovl tokens held by the contract are considered as collateral for the new position. The trading fee is calculated based on the total collateral, which includes:
- The
ovl tokens already present in the contract, and; - The additional
ovl tokens specified by the user;
Users are expected to compute the total collateral of the position before executing this call so they can set the correct allowance for the function.
However, an attacker could exploit this by donating small amounts of ovl tokens to the contract. Since the trading fee is based on the total collateral (which includes donated tokens), this could increase the required fee beyond the user's allowance. As a result, the transaction may fail due to insufficient allowance.
This failed transactions could result in a bad user experience.
Recommendation
To prevent unintended fee manipulation, consider tracking the ovl balance before and after calling _onUnwindPosition(...) and using only the actual unwound amount when computing collateral.
6.2 validMarkets(...) function may return incorrect results for uncached markets
The validMarkets mapping is used by the _checkIsValidMarket(...) function to cache valid markets.
/// @notice Mapping to check if an address is a valid market
mapping(address => bool) public validMarkets;
...
function _checkIsValidMarket(address _market) internal returns (bool) {
if (validMarkets[_market]) {
return true;
}
for (uint256 i = 0; i < authorizedFactories.length; i++) {
if (authorizedFactories[i].isMarket(_market)) {
validMarkets[_market] = true;
emit MarketValidated(_market);
return true;
}
}
return false;
}
The validMarkets mapping is public and a getter function for it is created automatically. While this function might appear to serve as a way to check whether a market is valid, it does not perform a full validation. Specifically, it returns false for valid markets that have not yet been cached by _checkIsValidMarket(...).
Recommendation
Consider changing the function to be private or internal if it is not intended to be used externally.
