Vader Protocol contest - Meta0xNull's results

Handle

Meta0xNull

Vulnerability details

Impact

Open TODOs can hint at programming or architectural errors that still need to be fixed.

Proof of Concept

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/math/VaderMath.sol#L80 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 More...

Tools Used

Manual Review

Recommended Mitigation Steps

Fix TODOs and Remove it

Handle

Meta0xNull

Vulnerability details

Impact

setRewardsDuration() Does Not Validate Input _rewardsDuration and thus Owner able to Input 0.

Then some Calculation in notifyRewardAmount() will fail since rewardsDuration = 0 now: rewardRate = reward.div(rewardsDuration) rewardRate = reward.add(leftover).div(rewardsDuration);

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L166-L173 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L130-L142

Tools Used

Manual Review

Recommended Mitigation Steps

In setRewardsDuration(), Add: require(_rewardsDuration > 0, "Reward Duation Can't Be Zero");

Handle

Meta0xNull

Vulnerability details

Impact

The local variable used as for loop index need not be initialized to 0 because the default value is 0. Avoiding this anti-pattern can save a few opcodes and therefore a tiny bit of gas.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/main/contracts/governance/GovernorAlpha.sol#L437 https://github.com/code-423n4/2021-11-vader/blob/main/contracts/tokens/vesting/LinearVesting.sol#L72

Tools Used

Manual Review

Recommended Mitigation Steps

Remove explicit 0 initialization of for loop index variable.

Before: for (uint256 i = 0; After: for (uint256 i;

Handle

Meta0xNull

Vulnerability details

Impact

If data can fit into 32 bytes, then you should use bytes32 datatype rather than string as it is much cheaper in solidity. Basically, Any fixed size variable in solidity is cheaper than variable size. That will save gas on the contract.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/main/contracts/governance/GovernorAlpha.sol#L30

Tools Used

Manual Review

Recommended Mitigation Steps

bytes32 public constant name = "Vader Governor Alpha";

Handle

Meta0xNull

Vulnerability details

Impact

Repeat SLOAD _pairs during the loop Save _pairs as pairData in Storage in loop Multiple SLOAD pairData within same loop

Storage SLOAD are more expensive than read local variables. The Gas Price Add Up during Loop is very very expensive.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/main/contracts/twap/TwapOracle.sol#L322-L368

Tools Used

Manual Review

Recommended Mitigation Steps

Before the Loop Start, SLOAD _pairs and caching it once in a local variable _pairs_temp. Then use local variable _pairs_temp in the loop.

PairData _pairs_temp = _pairs;

Handle

Meta0xNull

Vulnerability details

Impact

A wrong user input or wallets defaulting to the zero addresses for a missing input can lead to the contract needing to redeploy and wasted gas.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/main/contracts/staking-rewards/StakingRewards.sol#L40-L49

Tools Used

Manual Review

Recommended Mitigation Steps

requires Addresses is not Zero.

require(_owner != address(0) && _rewardsDistribution != address(0) && _rewardsToken != address(0) && _stakingToken != address(0), "Address Can't Be Zero")