Vader Protocol contest - pauliax's results

Handle

pauliax

Vulnerability details

Impact

Reliance that Chainlink Oracle decimals is 8:

  sumUSD += uint256(price) * (10**10);

Hardcoded values make the code harder to maintain, you can easily miss this when later deciding to deploy the same contracts on other chains, etc.

It would make the code more robust if you do not hardcode such values but fetch them directly. The downside is increased gas costs so at least you can extract this 10**10 as a constant to improve the readability and eliminate the evaluation of not changing value over and over again.

You can check that Chainlink interface contains decimals function: https://github.com/smartcontractkit/chainlink/blob/develop/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol

Recommended Mitigation Steps

To be honest, I think in practice It is very unlikely that these decimals will change but nevertheless wanted you to be informed about such a possibility.

Handle

pauliax

Vulnerability details

Impact

Open TODOs in Codebase

There are TODOs left in the code. While this does not cause any direct issue, it indicates a bad smell and uncertainty, especially seeing such todo messages:

// TODO: Uncomment prior to launch
// TBD

In previous reports, similar submissions were assigned a score of 'low' so I think it's a fair game to submit this as an issue here also. Reference: https://github.com/code-423n4/2021-09-swivel-findings/issues/67 and https://github.com/code-423n4/2021-10-tempus-findings/issues/39

TODOs: https://github.com/code-423n4/2021-11-vader/blob/main/contracts/tokens/USDV.sol#L38 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L157 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L209 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L265 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex-v2/pool/VaderPoolV2.sol#L400 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/pool/BasePool.sol#L163 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/pool/VaderPool.sol#L85 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/pool/VaderPool.sol#L93 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/router/VaderRouter.sol#L303 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/utils/GasThrottle.sol#L11 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/dex/math/VaderMath.sol#L80 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/twap/TwapOracle.sol#L125 https://github.com/code-423n4/2021-11-vader/blob/main/repo/vader-bond/contracts/VaderBond.sol#L299 https://github.com/code-423n4/2021-11-vader/blob/main/repo/vader-bond/contracts/VaderBond.sol#L336

Recommended Mitigation Steps

Consider fixing TODOs or removing them to ease the work of reviewers.

Handle

pauliax

Vulnerability details

Impact

Due to the asynchronous nature of blockchains, it is not advised to use toggle functions. In your case, you have function toggleQueue in contracts VaderPool and VaderPoolV2. A similar issue was reported in another contest and assigned a severity of low, you can read more details here: https://github.com/code-423n4/2021-06-realitycards-findings/issues/157

Recommended Mitigation Steps

Replace with a function where you can specify the exact value that you want to set.

Handle

pauliax

Vulnerability details

Impact

These functions do not check if the contract is already initialized or not, only if the new values are not empty. It transfers the ownership to the new gov but does not forbid calling this function again later. I am not entirely sure about the intentions here, maybe this is intentional but usually "init" keyword indicates that it is expected to be called only once. It does not cause any serious risk to the protocol itself but increases the reliance on the fairness of the governance and decreases trust from users because an ability to override the 'nativeAsset' or 'router' doesn't sound fair unless this is clearly mentioned somewhere.

Recommended Mitigation Steps

Depends on the intentions but my suggestion is to either forbid initializing it again or rename the function to avoid confusion.

Handle

pauliax

Vulnerability details

Impact

function setBondTerms relies on block times to be roughly 13.75s:

 // roughly 36 hours (262 blocks / hour)
 require(_input >= 10000, "vesting < 10000");

However, block times are likely to be reduced to 12s soon: https://twitter.com/TimBeiko/status/1456644168986419200

Recommended Mitigation Steps

Just wanted you to be informed about this. It is up to you to decide if this difference is significant enough to make code changes.

Handle

pauliax

Vulnerability details

Impact

the constructor of LinearVesting should validate that vesters are all unique addresses, otherwise, it will overwrite the existing vesting amount for that address.

Handle

pauliax

Vulnerability details

Impact

'immutable' greatly reduces gas costs. There are variables that do not change so they can be marked as immutable to greatly improve the gas costs. Examples of such variables are _name in BasePool _name, _vaderPool and _updatePeriod in TwapOracle, vader in XVader.

Handle

pauliax

Vulnerability details

Impact

Avoiding the initialization of loop index to 0 can save some gas. See for more details: https://github.com/code-423n4/2021-09-swivel-findings/issues/115

Handle

pauliax

Vulnerability details

Impact

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. There are some pretty long revert msgs, e.g.: "VaderPoolFactory::initialize: Incorrect Arguments". Consider shortening them.

Handle

pauliax

Vulnerability details

Impact

payout token exponent in function valueOfToken can be precalculated as this value is constant and does not change:

 value = _amount.mul(10**PAYOUT_TOKEN_DECIMALS).div(10**IERC20Metadata(_principalToken).decimals());

Extract 10**PAYOUT_TOKEN_DECIMALS as a constant.

Same situation here with 232 and 1010:

 uint32 blockTimestamp = uint32(block.timestamp % 2**32);
 sumUSD += uint256(price) * (10**10);

Handle

pauliax

Vulnerability details

Impact

queueActive in contract VaderPoolFactory is always false and there is no way to update this value so to save some gas consider getting rid of this state variable and just use a value of "false" when creating a new Pool.

Handle

pauliax

Vulnerability details

Impact

There are contracts that declare solidity compiler version >= 0.8 but still use SafeMath, e.g. UniswapV2Library, StakingRewards:

  using SafeMath for uint256;

Starting from 0.8 you no longer need to use an external SafeMath library as it is already baked in and used under the hood unless explicitly stated by using the 'unchecked' directive.

Handle

pauliax

Vulnerability details

Impact

There are some variables that are never used. Remove them to save some gas or use where they were intended. Examples are:

    uint256 internal constant _MIN_SWAPS_EXECUTED = 10;
    uint256 internal constant _DEFAULT_SWAPS_EXECUTED = 50_00;
    uint256 internal constant _QUEUE_SIZE = 100;

Handle

pauliax

Vulnerability details

Impact

The unchecked keyword can be applied in the following lines of code since there are statements before to ensure the arithmetic operations would not cause an integer underflow or overflow:

  return a > b ? a - b : b - a;

By the way, here because Solidity 0.8.9 is used, this desired overflow will fail:

 // subtraction overflow is desired
 uint32 timeElapsed = blockTimestamp - blockTimestampLast;
 // addition overflow is desired
 // counterfactual
 price0Cumulative +=
     uint256(FixedPoint.fraction(reserve1, reserve0)._x) *
     timeElapsed;