QuickSwap QuickPerps RewardDistributor Update Security Review Report

The RewardRouter implements transfer functionality for stake in the RewardTracker. The sender can send their using signalTransfer and consequently the receiver has to accept the transfer with acceptTransfer.

The acceptTransfer uses the _validateReceiver function, which checks that the receiver does not have an average stake or accrued rewards in the RewardTracker, otherwise the transfer will revert.

As such, it becomes possible to back-run the signalTransfer call and transfer a small amount using StakedQlp.transfer to the intended receiver. If the amount and time is enough, the receiver will receive a tiny amount of rewards (e.g. 1 Wei would be enough) and the legit transfer would get blocked.

RewardRouter.sol

function signalTransfer(address _receiver) external nonReentrant {
    _validateReceiver(_receiver);
    pendingReceivers[msg.sender] = _receiver;
}
 
function acceptTransfer(address _sender) external nonReentrant {
    address receiver = msg.sender;
    require(pendingReceivers[_sender] == receiver, "RewardRouter: transfer not signalled");
    delete pendingReceivers[_sender];
 
    _validateReceiver(receiver);
    uint256 qlpAmount = IRewardTracker(feeQlpTracker).depositBalances(_sender, qlp);
    if (qlpAmount > 0) {
        IRewardTracker(feeQlpTracker).unstakeForAccount(_sender, qlp, qlpAmount, _sender);
        IRewardTracker(feeQlpTracker).stakeForAccount(_sender, receiver, qlp, qlpAmount);
    }
}
 
function _validateReceiver(address _receiver) private view {
    require(IRewardTracker(feeQlpTracker).averageStakedAmounts(_receiver) == 0, "RewardRouter: feeQlpTracker.averageStakedAmounts > 0");
    require(!IRewardTracker(feeQlpTracker).hasCumulativeRewards(_receiver), "RewardRouter: feeQlpTracker.cumulativeRewards > 0");
}

RewardTracker.sol

function stakeForAccount(
    address _fundingAccount,
    address _account,
    address _depositToken,
    uint256 _amount
) external override nonReentrant {
    _validateHandler();
    _stake(_fundingAccount, _account, _depositToken, _amount);
}
 
function _validateHandler() private view {
    require(isHandler[msg.sender], "RewardTracker: forbidden");
}
 
function _stake(
    address _fundingAccount,
    address _account,
    address _depositToken,
    uint256 _amount
) private {
    require(_amount > 0, "RewardTracker: invalid _amount");
    require(isDepositToken[_depositToken], "RewardTracker: invalid _depositToken");
    IERC20(_depositToken).safeTransferFrom(_fundingAccount, address(this), _amount);
    _updateRewardsAll(_account);
    stakedAmounts[_account] = stakedAmounts[_account].add(_amount);
    depositBalances[_account][_depositToken] = depositBalances[_account][_depositToken].add(_amount);
    totalDepositSupply[_depositToken] = totalDepositSupply[_depositToken].add(_amount);
    _mint(_account, _amount);
}

StakedQlp.sol

function transfer(address _recipient, uint256 _amount) external returns (bool) {
    _transfer(msg.sender, _recipient, _amount);
    return true;
}
 
function _transfer(address _sender, address _recipient, uint256 _amount) private {
    require(_sender != address(0), "StakedQlp: transfer from the zero address");
    require(_recipient != address(0), "StakedQlp: transfer to the zero address");
    require(
        qlpManager.lastAddedAt(_sender).add(qlpManager.cooldownDuration()) <= block.timestamp,
        "StakedQlp: cooldown duration not yet passed"
    );
    IRewardTracker(feeQlpTracker).unstakeForAccount(_sender, qlp, _amount, _sender);
    IRewardTracker(feeQlpTracker).stakeForAccount(_sender, _recipient, qlp, _amount);
}

We would recommend to perhaps loosen the requirements in _validateReceiver, as the same transfer functionality is also implemented in StakedQlp without these requirements (except it being a 1-step instead of 2-step transferal). Another solution could be to disallow StakedQlp.transfer to pending receivers, but this would increase overhead for transfers by introducing an external call.

The function addRewardToken handles the adding and reactivating of reward tokens. There are 2 cases: if a reward token does not yet exist it will be added to the array and activated, if it already existed before, it will only be activated.

The total number of reward tokens in the array is also limited to 10 (MAX_ALL_REWARD_TOKENS).

However, the check for the array's length is done at the very beginning, which means that it assumes to be growing if the function is executed. This is not the case if an older reward token would be reactivated and thus it becomes impossible for a reward token to be reactivated if this limit is ever reached.

Instead, this check should only be done inside of the conditional if the reward is a new token and the array would indeed grow.

function addRewardToken(
    address _token
) external override onlyAdmin{
    require(allRewardTokens.length<MAX_ALL_REWARD_TOKENS,"RewardDistributor: too many rewardTokens");
    if (!rewardTokens[_token]) {
        bool isFound;
        uint256 length = allRewardTokens.length;
        for (uint256 i = 0; i < length; i++) {
            if(allRewardTokens[i] == _token){
                isFound = true;
                break;
            }
        }
        if(!isFound){
            allRewardTokens.push(_token);
            allTokens[_token] = true;
        }
        rewardTokens[_token] = true;
        rewardTokenCount++;
    }
}

The require(allRewardTokens.length<MAX_ALL_REWARD_TOKENS, "..") check should be moved to the case where the allRewardTokens array is pushed to. E.g.:

if(!isFound){
    require(allRewardTokens.length<MAX_ALL_REWARD_TOKENS, "..");
    allRewardTokens.push(_token);
    allTokens[_token] = true;
}

We have identified the following functions in the following contracts that check if the uint types are greater than zero using the > 0 operator:

1. _amountIn in OrderBook.sol:createSwapOrder() L344-372,
2. _sizeDelta in PositionManager.sol:_validateIncreaseOrder() L314-352,
3. size in PositionManager.sol:_validatePositionTime() L353-364,
4. _amount in QlpManager.sol:_addLiquidity() L209-230,
5. _qlpAmount in QlpManager.sol:_removeLiquidity() L232-255. However, using the != operator costs less gas, allowing for gas savings.

// 1.
function createSwapOrder(
    address[] memory _path,
    uint256 _amountIn,
    uint256 _minOut,
    uint256 _triggerRatio, // tokenB / tokenA
    bool _triggerAboveThreshold,
    uint256 _executionFee,
    bool _shouldWrap,
    bool _shouldUnwrap
) external payable nonReentrant {
    require(_path.length == 2 || _path.length == 3, "OB: invalid _path.length");
    require(_path[0] != _path[_path.length - 1], "OB: invalid _path");
    require(_path[0] != usdq && _path[_path.length - 1] != usdq, "OB: invalid token");
    require(_amountIn > 0, "OB: invalid _amountIn");
    require(_executionFee >= minExecutionFee, "OB: insufficient execution fee");
    // always need this call because of mandatory executionFee user has to transfer in ETH
    _transferInETH();
    if (_shouldWrap) {
        require(_path[0] == weth, "OB: only weth could be wrapped");
        require(msg.value == _executionFee.add(_amountIn), "OB: incorrect value transferred");
    } else {
        require(msg.value == _executionFee, "OB: incorrect execution fee transferred");
        IRouter(router).pluginTransfer(_path[0], msg.sender, address(this), _amountIn);
    }
    _createSwapOrder(msg.sender, _path, _amountIn, _minOut, _triggerRatio, _triggerAboveThreshold, _shouldUnwrap, _executionFee);
}

// 2.
function _validateIncreaseOrder(address _account, uint256 _orderIndex) internal view {
    (
        address _purchaseToken,
        uint256 _purchaseTokenAmount,
        address _collateralToken,
        address _indexToken,
        uint256 _sizeDelta,
        bool _isLong,
        , // triggerPrice
        , // triggerAboveThreshold
        // executionFee
    ) = IOrderBook(orderBook).getIncreaseOrder(_account, _orderIndex);
 
    _validateMaxGlobalSize(_indexToken, _isLong, _sizeDelta);
 
    if (!shouldValidateIncreaseOrder) { return; }
 
    // shorts are okay
    if (!_isLong) { return; }
 
    // if the position size is not increasing, this is a collateral deposit
    require(_sizeDelta > 0, "PositionManager: long deposit");
 
    IVault _vault = IVault(vault);
    (uint256 size, uint256 collateral, , , , , , ) = _vault.getPosition(_account, _collateralToken, _indexToken, _isLong);
 
    // if there is no existing position, do not charge a fee
    if (size == 0) { return; }
 
    uint256 nextSize = size.add(_sizeDelta);
    uint256 collateralDelta = _vault.tokenToUsdMin(_purchaseToken, _purchaseTokenAmount);
    uint256 nextCollateral = collateral.add(collateralDelta);
 
    uint256 prevLeverage = size.mul(BASIS_POINTS_DIVISOR).div(collateral);
    // allow for a maximum of a increasePositionBufferBps decrease since there might be some swap fees taken from the collateral
    uint256 nextLeverageWithBuffer = nextSize.mul(BASIS_POINTS_DIVISOR + increasePositionBufferBps).div(nextCollateral);
 
    require(nextLeverageWithBuffer >= prevLeverage, "PositionManager: long leverage decrease");
}

// 3.
function _validatePositionTime(
    address _account,
    address _collateralToken,
    address _indexToken,
    bool _isLong
) private view{
    IVault _vault = IVault(vault);
    (uint256 size, , , , , , , uint lastIncreasedTime) = _vault.getPosition(_account, _collateralToken, _indexToken, _isLong);
    require(size > 0, "PositionManager: empty position");
    uint256 minDelayTime = partnerMinStayingOpenTime[_account]>0 ? partnerMinStayingOpenTime[_account] : minStayingOpenTime;
    require(lastIncreasedTime.add(minDelayTime) <= block.timestamp, "PositionManager: min delay not yet passed");
}

// 4.
function _addLiquidity(address _fundingAccount, address _account, address _token, uint256 _amount, uint256 _minUsdq, uint256 _minQlp) private returns (uint256) {
    require(_amount > 0, "QlpManager: invalid _amount");
 
    // calculate aum before buyUSDQ
    uint256 aumInUsdq = getAumInUsdq(true);
    uint256 qlpSupply = IERC20(qlp).totalSupply();
 
    IERC20(_token).safeTransferFrom(_fundingAccount, address(vault), _amount);
    uint256 usdqAmount = vault.buyUSDQ(_token, address(this));
    require(usdqAmount >= _minUsdq, "QlpManager: insufficient USDQ output");
 
    uint256 mintAmount = aumInUsdq == 0 ? usdqAmount : usdqAmount.mul(qlpSupply).div(aumInUsdq);
    require(mintAmount >= _minQlp, "QlpManager: insufficient QLP output");
 
    IMintable(qlp).mint(_account, mintAmount);
 
    lastAddedAt[_account] = block.timestamp;
 
    emit AddLiquidity(_account, _token, _amount, aumInUsdq, qlpSupply, usdqAmount, mintAmount);
 
    return mintAmount;
}

// 5.
function _removeLiquidity(address _account, address _tokenOut, uint256 _qlpAmount, uint256 _minOut, address _receiver) private returns (uint256) {
    require(_qlpAmount > 0, "QlpManager: invalid _qlpAmount");
    require(lastAddedAt[_account].add(cooldownDuration) <= block.timestamp, "QlpManager: cooldown duration not yet passed");
 
    // calculate aum before sellUSDQ
    uint256 aumInUsdq = getAumInUsdq(false);
    uint256 qlpSupply = IERC20(qlp).totalSupply();
 
    uint256 usdqAmount = _qlpAmount.mul(aumInUsdq).div(qlpSupply);
    uint256 usdqBalance = IERC20(usdq).balanceOf(address(this));
    if (usdqAmount > usdqBalance) {
        IUSDQ(usdq).mint(address(this), usdqAmount.sub(usdqBalance));
    }
 
    IMintable(qlp).burn(_account, _qlpAmount);
 
    IERC20(usdq).transfer(address(vault), usdqAmount);
    uint256 amountOut = vault.sellUSDQ(_tokenOut, _receiver);
    require(amountOut >= _minOut, "QlpManager: insufficient output");
 
    emit RemoveLiquidity(_account, _tokenOut, _qlpAmount, aumInUsdq, qlpSupply, usdqAmount, amountOut);
 
    return amountOut;
}

Replace the > 0 checks with != 0 to optimize for gas usage.

The function addRewardToken makes use of a for-loop to check if a token ever existed before pushing it to the allRewardTokens array.

However, this is unnecessary because the allTokens mapping is only every set when the reward token is pushed to array. The array never shrinks and the mapping is never set to false, so it can be used to lookup the existence in constant time.

function addRewardToken(
    address _token
) external override onlyAdmin{
    require(allRewardTokens.length<MAX_ALL_REWARD_TOKENS,"RewardDistributor: too many rewardTokens");
    if (!rewardTokens[_token]) {
        bool isFound;
        uint256 length = allRewardTokens.length;
        for (uint256 i = 0; i < length; i++) {
            if(allRewardTokens[i] == _token){
                isFound = true;
                break;
            }
        }
        if(!isFound){
            allRewardTokens.push(_token);
            allTokens[_token] = true;
        }
        rewardTokens[_token] = true;
        rewardTokenCount++;
    }
}

The addRewardToken should be optimised by removing the for-loop and make use of the allTokens mapping. For example (and by also taking the recommendation of issue QSWP-37: Reactivating reward token impossible when total limit is reached into account):

function addRewardToken(
    address _token
) external override onlyAdmin{
    if (!rewardTokens[_token]) {
        if (!alltokens[_token]) {
            require(allRewardTokens.length < MAX_ALL_REWARD_TOKENS,"RewardDistributor: too many rewardTokens");
            allRewardTokens.push(_token);
            allTokens[_token] = true;
        }
        rewardTokens[_token] = true;
        rewardTokenCount++;
    }
}

The parameters of the function setMaxGlobalSizes can be marked as calldata parameters instead of memory in favour of gas saving.

function setMaxGlobalSizes(
    address[] memory _tokens,
    uint256[] memory _longSizes,
    uint256[] memory _shortSizes
) external onlyAdmin {
    for (uint256 i = 0; i < _tokens.length; i++) {
        address token = _tokens[i];
        maxGlobalLongSizes[token] = _longSizes[i];
        maxGlobalShortSizes[token] = _shortSizes[i];
    }
    emit SetMaxGlobalSizes(_tokens, _longSizes, _shortSizes);
}

We found that the ReentrancyGuard library is imported and implemented by a number of contracts. However, the modifier nonReentrant is not used by some of the contracts:

1. StakedQlp.sol
2. RewardDistributor.sol The ReentrancyGuard import should be removed.

import "../libraries/utils/ReentrancyGuard.sol";

This update adds new functionality to the RewardDistributor, such as the addition of multiple reward tokens.

The new functions that add and remove reward tokens update the state, however they do not emit events to notify off-chain watchers of changes.

function addRewardToken(
    address _token
) external override onlyAdmin{
    require(allRewardTokens.length<MAX_ALL_REWARD_TOKENS,"RewardDistributor: too many rewardTokens");
    if (!rewardTokens[_token]) {
        bool isFound;
        uint256 length = allRewardTokens.length;
        for (uint256 i = 0; i < length; i++) {
            if(allRewardTokens[i] == _token){
                isFound = true;
                break;
            }
        }
        if(!isFound){
            allRewardTokens.push(_token);
            allTokens[_token] = true;
        }
        rewardTokens[_token] = true;
        rewardTokenCount++;
    }
}
 
function removeRewardToken(
    address _token
) external override onlyAdmin{
    if(rewardTokens[_token]){
        require(tokensPerInterval[_token] == 0,"RewardDistributor: tokensPerInterval must be zero");
        delete rewardTokens[_token];
        rewardTokenCount--;
    }
}

Add and emits events for adding and removing reward tokens.