# Security Audit of `VELA` contracts.

## Conclusion

This audit was made by
Auditor: Vladimir Smelov <[e-mail]>.
Date: 2022DEC22

No security issues found.

## Scope

repository: https://github.com/VelaExchange/vela-contracts

initial audit commit: 6034f43cb18c705f2daa27a0008ac375ba124b16
reaudit commit: b3200f08472ab6db8e62aa3d9a766b4dbe7b70cb

## Methodology

1. Blind audit. Understand the structure of the code without reading any docs.
2. Ask questions to developers.
3. Run static analyzers.
4. Find problems with:
- backdoors;
- bugs;
- math;
- potential leaking of funds;
- potential locking of the contract;
- validate arguments and events;
- others.

## Result

#### CRITICAL-1 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L219

wrong argument, it should be _account

##### Status.
FIXED

______

#### CRITICAL-2 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L647

wrong math, some feeReserves are never withdrawn by anyone.

To take the intuitive feeliing why it does happen, let's imagine that feeRewardBasisPoints=100% (stakers receive full reward, and owner receives nothing), and Alice stake and unstake some amount of tokens, then she will obviously receive nothing (see how user.lastFeeReserves is updated) and the owner will also receive nothing, so stakeFee tokens will be just lost on the contract. The same problem is still in place if feeReward<100%.

It makes whole fee sharing system unfair, because user fee reward is really dependent on which moment they claim fee reward. Because the totalVLP part is volatile.

Also let's take a look on a simple python script with simplified Math from the contract:

# let 1vlp = 1usd
# let _rewardToken = USDT
# let _token = USDT

from collections import Counter

staking_fee = 0.1
reward_fee = 0.7
fee_reserves = 0
total_VLP = 0
user_amount = Counter()
last_fee_reserves = Counter()
received_rewards = Counter()

owner_last_fee_reserves = 0

balances = Counter()
balances['aaa'] = 100
balances['bbb'] = 100
balances['ccc'] = 100


def transferFrom(_from, _to, amount):
    balances[_from] -= amount
    assert balances[_from] >= 0
    balances[_to] += amount


def update_reward(user):
    global fee_reserves, total_VLP, owner_last_fee_reserves
    if total_VLP == 0: return
    r = reward_fee * (fee_reserves-last_fee_reserves[user]) * user_amount[user] / total_VLP
    transferFrom('this', user, r)
    received_rewards[user] += r
    print(f'update_reward {user=} {r=}')


def stake(user, amount):
    global fee_reserves, total_VLP, owner_last_fee_reserves
    update_reward(user)
    afterFeeAmount = amount * (1 - staking_fee)
    user_amount[user] += afterFeeAmount
    fee_reserves += (amount - afterFeeAmount)
    last_fee_reserves[user] = fee_reserves
    total_VLP += afterFeeAmount
    transferFrom(user, 'this', amount)
    print(f'stake({user=}, {amount=}) {afterFeeAmount}')


def unstake(user, amount):
    global fee_reserves, total_VLP, owner_last_fee_reserves
    update_reward(user)
    total_VLP -= amount
    afterFeeAmount = amount * (1 - staking_fee)
    user_amount[user] -= amount
    fee_reserves += (amount - afterFeeAmount)
    last_fee_reserves[user] = fee_reserves
    transferFrom('this', user, afterFeeAmount)
    print(f'unstake({user=}, {amount=}) {afterFeeAmount}')


def owner_withdraw_fees():
    global fee_reserves, total_VLP, owner_last_fee_reserves
    r = (fee_reserves - owner_last_fee_reserves) * (1 - reward_fee)
    owner_last_fee_reserves = fee_reserves
    transferFrom('this', 'owner', r)
    received_rewards['owner'] += r
    print(f'owner_withdraw_fees() - {r=}')


def main0():
    global fee_reserves, total_VLP, owner_last_fee_reserves
    stake('aaa', 1)
    stake('bbb', 1)
    stake('ccc', 1)
    owner_withdraw_fees()
    fee_reserves += 1000000
    balances['this'] += 1000000
    unstake('aaa', user_amount['aaa'])
    unstake('bbb', user_amount['bbb'])
    unstake('ccc', user_amount['ccc'])
    owner_withdraw_fees()


def main1():
    stake('aaa', 100)
    stake('bbb', 100)
    stake('ccc', 100)
    owner_withdraw_fees()
    unstake('bbb', user_amount['bbb'])
    unstake('ccc', user_amount['ccc'])
    unstake('aaa', user_amount['aaa'])
    owner_withdraw_fees()


def main2():
    stake('aaa', 100)

    stake('bbb', 100)
    unstake('aaa', 0.000001)  # trigger update_reward

    stake('ccc', 100)
    unstake('aaa', 0.000001)  # trigger update_reward

    unstake('bbb', user_amount['bbb'])
    unstake('aaa', 0.000001)  # trigger update_reward

    unstake('ccc', user_amount['ccc'])
    unstake('aaa', 0.000001)  # trigger update_reward

    unstake('aaa', user_amount['aaa'])
    owner_withdraw_fees()


def main3():
    stake('aaa', 100)
    unstake('aaa', user_amount['aaa'])
    owner_withdraw_fees()


if __name__ == '__main__':
    # main1()  # fee_reserves=57.0, sum(received_rewards.values())=49.18333333333334, received_rewards[aaa]=26.6
    # main2()  # fee_reserves=57.0, sum(received_rewards.values())=37.86666680083334, received_rewards[aaa]=15.28
    # main3()
    main0()
    print(f'{balances=}')
    print(f'{fee_reserves=}')
    print(f'{sum(received_rewards.values())=}')
    print(f'{received_rewards=}')
    print(f'{user_amount=}')

##### Status.
FIXED - feeReserves is not used anymore
but there is some new logic to distribute fees

______

#### CRITICAL-3 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/VaultUtils.sol#L278

Anyone can set this important setting. The access should be restricted.

##### Status.
FIXED - setFundingRateFactor was removed

______

#### CRITICAL-4 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/VaultPriceFeed.sol#L57

you should compare `p>0`, since uint256(-1)=type(uint256).max

proof - https://gist.github.com/vsmelov/13ab37cc7baf2954f37e9c44d7b12f97

##### Status.
FIXED
function latestAnswer returns uint256 instead of int256.

______

#### CRITICAL-5 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/staking/TokenFarm.sol#L438

wrong Math

imagine if we have tierLevels=[1000, 2000, 3000],
tierPercents=[a,b,c]

this function will make such calculations:

IF 1000 <= stakedAmount and stakedAmount < 2000:
tierPercent = b

ELSE IF stakedAmount > 3000:
tierPercent = c

ELSE:
tierPercent = BASIS_POINTS_DIVISOR

so for range [2000, 3000) it will BASIS_POINTS_DIVISOR

it looks incorrect.

Moreover, if tierLevels.len()==1, for loop will never run.

##### ReAudit commentary

let's say
tierLevels = [10, 20]
tierPercent = [x, y]

basicly the calculations should be

if (10 <= amount && amount < 20) {
tierPercent = x
} else if (amount >= 20) {
tierPercent = y
} else {
tierPercent = BASIS_POINTS_DIVISOR
}

but for the latest code

uint256 tierPercent = BASIS_POINTS_DIVISOR;
for (uint256 i = 1; i < tierLevels.length; i++) {
if (tierLevels[i - 1] <= user.amount && user.amount < tierLevels[i]){
tierPercent = tierPercents[i - 1];
}
if (i == tierLevels.length - 1) {
if (user.amount > tierLevels[i])
{
tierPercent = tierPercents[i];
}
}
}

there will be only one iteration in the loop
i = 1

you can unwrap it as

tierPercent = BASIS_POINTS_DIVISOR
if (10 <= amount && amount < 20) {
tierPercent = x
}
if (amount > 20) {
tierPercent = y
}

I see several problems here

1) HIGH what if amount==20? then tierPercent = BASIS_POINTS_DIVISOR, it does not look correct, intervals should border value

2) LOW conditions for i==length-1 are checking twice, first
if (tierLevels[i - 1] <= user.amount && user.amount < tierLevels[i])
and
if (user.amount > tierLevels[i])

3) LOW it makes sense to break iteration after you found a match to not waste the gas and dont go to next iterations

4) LOW you check
if (i == tierLevels.length - 1)
on every iteration
but you can just do it once

5) LOW you read storage value tierLevels.length on the every iteration step

6) LOW safeMath for solc 0.8.X could be disabled for this part of code to save gas

consider refactoring as

uint256 length = tierLevels.length; // vsm: read storage once
uint256 tierPercent = BASIS_POINTS_DIVISOR;
if (length == 0) return tierPercent; // vsm: skip checks for empty intervals
unchecked { // vsm: overflow/underflow for i+1, length-1 isn't possible, so disable default safeMath behaviour of solc 0.8.X
if (user.amount >= tierLevels[length-1]) { // vsm: check the final condition once
tierPercent = tierPercents[length-1]; // vsm: note, I changed > to >=
} else { // vsm: search over intervals
for (uint256 i = 0; i < length-1; ++i) {
if (tierLevels[i] <= user.amount && user.amount < tierLevels[i+1]){
tierPercent = tierPercents[i];
break; // vsm: break on the first successful match
}
}
}
}

##### Status.
FIXED

this looks correct

    function getTier(
        uint256 _pid,
        address _account
    ) external view override returns (uint256) {
        UserInfo storage user = userInfo[_pid][_account];
        if (tierLevels.length == 0 || user.amount < tierLevels[0]) {
            return BASIS_POINTS_DIVISOR;
        }
        unchecked {
            for (uint16 i = 1; i != tierLevels.length; ++i) {
                if (user.amount < tierLevels[i]) {
                    return tierPercents[i - 1];
                }
            }
            return tierPercents[tierLevels.length - 1];
        }
    }

BUT it's better to replace uint16 with uint256 because it's more gas efficient.

______

#### MAJOR-1 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/BetaTrading.sol#L23

There is no restriction to call this function, I can call it 10000 times from different accounts.

Consider creation of whitelist.

##### Status.
FIXED - the contract was removed

______

#### MAJOR-2 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/BetaTrading.sol#L47

it may give not correct accounts, since tokens could be burned or new tokens could be minted. The purpose of this function is unclear.

##### Status.
FIXED - the contract was removed

______

#### MAJOR-3 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L346

how should it work in very begin when the chainlink does not know the price of VLP token?

##### Status.
FIXED with the usage of DEFAULT_VLP_PRICE

______

#### MAJOR-4 (TODO).

At
- https://github.com/VelaExchange/vela-contracts/blob/b3200f08472ab6db8e62aa3d9a766b4dbe7b70cb/contracts/core/Vault.sol#L497
- https://github.com/VelaExchange/vela-contracts/blob/b3200f08472ab6db8e62aa3d9a766b4dbe7b70cb/contracts/core/Vault.sol#L616

what if user transferred our VLP? VLP is transferable token.

if ALICE calls this flow

- stake receiving 100 VLP
- transfer to Bob 100 VLP
- unstake(_vlpAmount=100VLP)
this transaction will fail, neither Bob will not be able to call unstake.
No one will be able to unstake (until Alice will buy back 100VLP)
is it how it is supposed to be?

##### Status.
TODO

##### Client's comment

> I see your point on transferring VLP. However, we want users to be able to stake VLP to earn extra rewards. Any suggestions on how we can have it remain transferrable but prevent this?

##### Re-audit

If VLP is a usual ERC20 token, then anybody can create DEX POOL, e.g. USDT-VLP.

Let's say, we have 1 VLP = 100 USDT DEX POOL

1) Alice calls Vault.stake 1000 USDT for minted 10 VLP
2) Alice calls DEX.swap 10 VLP to 1000 USDT on DEX
3) Alice calls Vault.stake 1000 USDT for minted 10 VLP
4) Alice calls DEX.swap 10 VLP to 1000 USDT on DEX
5) ...

in fact, because of the fees and slippages on every step, the minted amount will be smaller and the price move on the DEX will make the VLP price lower.

Also, in the end, the price on DEX will not match Vault.getVLPPrice() function.

Is it OK?

______

#### MAJOR-5 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L650

totalVLP is changed often the calculations are not fair

##### Status.
FIXED - updateReward function was removed

______

#### MAJOR-6 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L654

why are you so sure that there will be enough _rewardToken? What should happen if the contract has not enough reward token on the balance?

##### Status.
ACKNOWLEDGED

##### Client's comment

> We will ensure enough reward token is there

______

#### MAJOR-7 (TODO).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/VaultPriceFeed.sol#L50

what should it return if chainlink is offline, consider revert always.

##### Status.
TODO

##### Client's comment

> We are not using chainlink, we are using custom price feed

##### Re-audit

even if you use FastPriceFeed contract it's possible that admin who must calls setLatestAnswer will go offline
and you will have very old data in answers and latestAts

consider adding a check of the latest timestamp update to latestAnswer function like

    function latestAnswer() external view override returns (uint256) {
        require(block.timestamp - latestAts[roundId] > 1 minute, "price feed is online");
        return answer;
    }

even in this case price may move drastically in a second, it should be explained in the contract NatSpec what should happen if the token price moves significantly BEFORE priceFeed was updated.

______

#### MAJOR-8 (TODO).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/staking/TokenFarm.sol#L368

what about reflactionary tokens?

##### Status.
TODO

##### Client's comment

> Why not, it is correct

##### Re-audit

This note about the possibility of the usage of deflationary tokens (or tokens with fee on transfer) is still not 100% for me.

For example in `stake` function you call

    function _transferIn(
        address _account,
        address _token,
        uint256 _amount
    ) internal {
        IERC20(_token).safeTransferFrom(_account, address(this), _amount);
    }

but it does not mean that address(this) will actually receive _amount of tokens. Because it may have deflationary mechanism under the hood or just have fee (as even in USDT).

The manner how such tokens will be handled should be described in details in contract comments.

##### Client's comment

> deflationary tokens - we are responsible for loading the tokenfarms, so we can avoid those, but is there an easy way to exclude them from ever being loaded?

##### Re-audit

No, in fact you cannot avoid it.

You should be aware that your calculations may break on usage of a deflationary token or even a feeOnTransfer token.

To make calculations more robust you may modify your functions like

    function _transferIn(
        address _account,
        address _token,
        uint256 _amount
    ) internal returns(uint256 actualAmount) {
        uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
        IERC20(_token).safeTransferFrom(_account, address(this), _amount);
        actualAmount = IERC20(_token).balanceOf(address(this)) - balanceBefore;
    }

or

    function _transferIn(
        address _account,
        address _token,
        uint256 _amount
    ) internal {
        uint256 balanceBefore = IERC20(_token).balanceOf(address(this));
        IERC20(_token).safeTransferFrom(_account, address(this), _amount);
        require(IERC20(_token).balanceOf(address(this)) - balanceBefore == _amount, "failed transfer");
    }

______

#### MAJOR-9 (FIXED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/staking/ComplexRewardPerSec.sol#L154

it's difficult to calculate preciesly proper eth amount, this call may often fail. consider transferring rest back

##### Status.
FIXED - isNative branch was removed

______

#### MAJOR-10 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/staking/ComplexRewardPerSec.sol#L178

declare emergency state

The contract owner may intentionally or unintentionally break the contract state if call these functions outside of real emergency situation.

##### Status.
ACKNOWLEDGED

##### Client's comment

> it's correct

-------

#### MAJOR-11 (NEW).

At:

- https://github.com/VelaExchange/vela-contracts/blob/b3200f08472ab6db8e62aa3d9a766b4dbe7b70cb/contracts/core/SettingsManager.sol#L248

you have a function to set fundingInterval

    function setFundingInterval(uint256 _fundingInterval) external onlyGov {
        require(
            _fundingInterval >= MIN_FUNDING_RATE_INTERVAL,
            "fundingInterval should be greater than MIN"
        );
        require(
            _fundingInterval <= MAX_FUNDING_RATE_INTERVAL,
            "fundingInterval should be smaller than MAX"
        );
        fundingInterval = _fundingInterval;  //xxx MAJOR should be immutable check updateCumulativeFundingRate
        emit SetFundingInterval(fundingInterval);
    }

the updating of this setting by itself should not affect any user profits.

But let's say, you have fundingInterval = 8 hours
and you call setFundingInterval(4 hours)

On the next call of `updateCumulativeFundingRate`

you will have

        cumulativeFundingRates[_token][_isLong] += getNextFundingRate(
            _token,
            _isLong
        );

and

    function getNextFundingRate(
        address _token,
        bool _isLong
    ) internal view returns (uint256) {
        uint256 intervals = (block.timestamp -
            lastFundingTimes[_token][_isLong]) / fundingInterval;
        if (vault.poolAmounts(_token, _isLong) == 0) {
            return 0;
        }
        return
            ((
                fundingRateFactor[_token][_isLong] > 0
                    ? fundingRateFactor[_token][_isLong]
                    : DEFAULT_FUNDING_RATE_FACTOR
            ) *
                vault.reservedAmounts(_token, _isLong) *
                intervals) / vault.poolAmounts(_token, _isLong);
    }

so intervals will be twice bigger, (assumpting fundingRateFactor did not change).
and it will lead to twice bigger funding rate.

##### Recommendation

Consider declaring `fundingInterval` as immutable variable, to prevent this miscalculations.
Or consider adding a check for `fundingRateFactor` to be updated according to the `fundingInterval` change.

______

#### WARNING-1 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol

why do we need variable totalVLP
why not use VLP.balanceOf(address(this))
why store the same data in two places?

##### Status.
ACKNOWLEDGED

______

#### WARNING-2 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L54

unclear purpose, prices of locked token may change

##### Status.
ACKNOWLEDGED

##### Client's comment

> Unclear. What you are asking

##### Re-audit

totalUSDC does not say how many value in USD are stored on the Vault balance, because the price of every token in USD is always changing. Be careful about it. The fact that you allow only stable coins as a staked token should be mentioned in the contract comments.
Be aware that sometimes even stablecoins could drastically change their price ( check USTC price plot - https://coinmarketcap.com/currencies/terrausd/ )
if you allow e.g. 3 stablecoins and one of them will fail (as USTC did), it may break your project.

##### Client's comment

> totalUSDC means the rewardable USDC amount + deposited USDC for stake.
so with this way, we can distribute rewards to VLP holders,
It is not for the USD are stored on the Vault balance.

______

#### WARNING-3 (TODO).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/VaultPriceFeed.sol#L60

how will it work with tokens if they have decimals>>30?
will it loose the accuracy?

##### Status.
TODO

______

#### WARNING-4 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/staking/libraries/BoringERC20.sol#L71

this fallback is dangerous, not all erc20 tokens with no public decimals() method has 18 digits in implementation. Consider revert.

##### Status.
function safeDecimals(IBoringERC20 token) internal view returns (uint8) {
(bool success, bytes memory data) = address(token).staticcall(
abi.encodeWithSelector(SIG_DECIMALS)
);
return success && data.length == 32 ? abi.decode(data, (uint8)) : 18; //xx CRITICAL this fallback is dangerous
}

it still looks dangerous for me

Im pretty sure I will be able to find some token which has no implementation of decimals() method, but use some other number rather than 18 under the hood

https://eips.ethereum.org/EIPS/eip-20

there is no exact note that if a token has no "decimals()" method it shoud be considered as having 18 as a default decimals

for example, this contract - https://github.com/OpenZeppelin/openzeppelin-contracts/blob/v4.2.0/contracts/token/ERC20/extensions/ERC20Wrapper.sol
if someone use it as a template to wrap USDC (with decimals=6) and create wUSDC

BoringERC20(wUSDC).safeDecimals() == 18
but in reality it's 6

this is potentially dangerous, dont rely on something if it is not exactly given 🙂

the severety of the issue could be decreased from CRITICAL ot WARNING, because its not used or any financial calculations as far as i can see
but be aware

##### Status.
ACKNOWLEDGED

______

#### WARNING-5 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/staking/ComplexRewardPerSec.sol#L144

restrict the window to be 7 days to match the comment about 1 year at the beginning of the contract.

##### Status.
ACKNOWLEDGED

##### Client's comment
> it's correct

##### Re-audit

In fact, nothing prevents the owner to place other intervals rather than 7 days so taking into account rewardInfoLimit=52 it will be different from 1 year

______

#### WARNING-6 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/oracle/PriceFeed.sol#L44

Why last arguments are always zero? What is their purpose?

##### Client's comment

> We use zeros to match interface.

##### Status.
ACKNOWLEDGED

______

#### WARNING-7 (ACKNOWLEDGED).

At
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L400

stuck user deposits if token was disabled after stake.

Owner may intentionally or unintentionally block users' tokens.

##### Status.
ACKNOWLEDGED

-------

#### WARNING-8 (NEW).

At:

- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L92
- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L114

you have a code like

    event NewOrder(
        bytes32 key,
        address indexed account,
        address indexToken,
        bool isLong,
        uint256 posId,
        uint256 positionType,
        OrderStatus orderStatus,
        uint256[] triggerData
    );

the key is not indexed, however other events key attribute are indexed.

Consider declaring as an indexed attribute.

-------

#### WARNING-9 (NEW).

At:

- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L834

you have a code like

        if (position.size == 0) {
            position.averagePrice = price;
        }

        if (position.size > 0 && _sizeDelta > 0) {
            position.averagePrice = priceManager.getNextAveragePrice(
                _indexToken,
                position.size,
                position.averagePrice,
                _isLong,
                price,
                _sizeDelta
            );
        }

it's not clear, is it ok to keep averagePrice without any change if `position.size > 0` but `_sizeDelta==0`? It seels like a possible case, since `_sizeDelta` is given by a user.

-------

#### WARNING-10 (NEW).

At:

- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L242

you have a confusing code

        if (order.positionType == POSITION_TRAILING_STOP) {
            order.status = OrderStatus.FILLED;
            order.positionType = POSITION_MARKET;
        } else {
            order.status = OrderStatus.CANCELED;
        }

it's not clear why the order.status is set to FILLED however in fact, the order was manually cancelled.

-------

#### WARNING-11 (NEW).

At:

- https://github.com/VelaExchange/vela-contracts/blob/6034f43cb18c705f2daa27a0008ac375ba124b16/contracts/core/Vault.sol#L322

you have a confusing error message

        require(
            (settingsManager.checkDelegation(_account, msg.sender)) &&
                _amount > 0,
            "Vault: zero amount not allowed for deposit"
        );

if delegation was not enabled for this msg.sender, he will get confusing error message "Vault: zero amount not allowed for deposit".

Consider splitting this require in 2 with a clear error messages.