Vader Protocol contest - ye0lde's results

Handle

ye0lde

Vulnerability details

Impact

Code clarity

Proof of Concept

"only callable by guardian" is incorrect https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L698

"less than {proposalMaxOperations}" to "less than or equal" https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L347

Struct member description out of order: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L68

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Correct the comments if the suggestions are valid.

Handle

ye0lde

Vulnerability details

Impact

It's not clear what this function should allow to happen. The comment states: "the new maximum supply must be greater than the current one".

But the require statement only checks that the new maximum supply is >= totalSupply.

So the way the code is written now maximum supply can go up or down as long as its >= totalSupply.

And if that is correct then can maximum supply really be equal to totalSupply? The revert message is not really clear.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/tokens/Vader.sol#L206-L224

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Clarify the comment/revert message if the code is correct. Correct the code otherwise.

Handle

ye0lde

Vulnerability details

Impact

Removing unused named return variables can reduce gas usage and improve code clarity.

Proof of Concept

Unused named returns https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L250 https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/tokens/vesting/LinearVesting.sol#L101

Used but not needed (replace with a return statement) https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/repo/vader-bond/contracts/Treasury.sol#L55-L57

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Remove the unused or unneeded named returns

Handle

ye0lde

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. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I can see that the sponsor is committed to verbose revert strings as almost every revert string is > 32 bytes but I wanted to at least mention this issue.

Proof of Concept

Almost every revert string in the project is > 32 bytes.

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Consider shortening the revert strings to fit in 32 bytes or using custom errors (v0.8.4 or greater) in the future.

Handle

ye0lde

Vulnerability details

Impact

Variables are being assigned their default value which is unnecessary. Removing the assignment will save gas when deploying.

Proof of Concept

https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/staking-rewards/StakingRewards.sol#L26-L27

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Remove the assignments.

Or if you feel it is important to show the default assignment will occur then replace the assignments with a comment.

Handle

ye0lde

Vulnerability details

Impact

Using existing local variables instead of reading state variables will save gas by converting SLOADs to MLOADs.

Proof of Concept

delay_ can be used here: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/Timelock.sol#L150

pendingAdmin_ can be used here: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/Timelock.sol#L186

_paused can be used here: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/staking-rewards/Pausable.sol#L30 https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/staking-rewards/Pausable.sol#L35

_rewardsDuration can be used here: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/staking-rewards/StakingRewards.sol#L172

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

See Proof of Concept

Handle

ye0lde

Vulnerability details

Impact

In constructors and elsewhere a "pool" address is being passed as a parameter and it is checked for the zero address. This check is not needed if there is an immediate call to "transferOwnership" as it will revert on an attempt to transfer ownership to the zero address.

Removing the redundant check will save gas and improve code clarity.

Proof of Concept

Remove the require: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/synths/SynthFactory.sol#L13-L16 https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex-v2/wrapper/LPWrapper.sol#L13-L16

Remove one of the conditions in the require: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/dex/pool/VaderPoolFactory.sol#L103 https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/reserve/VaderReserve.sol#L69

Could also do it here but it is less clear: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/tokens/Vader.sol#L158

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Consider removing the zero address check. A comment could be added that "transferOwnership" reverts on a zero address.

Handle

ye0lde

Vulnerability details

Impact

The proposal "feeAmount" variable is set in the constructor and elsewhere without any boundary checking. While the proposer has to approve the feeAmount before calling it may have been incorrectly set too low allowing griefing attacks, etc or set too high preventing legitimate proposals.

Proof of Concept

Initialized in Constructor: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L215

Set: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L545

Used: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L373

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

Consider adding boundary checks on feeAmount here: https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L215 https://github.com/code-423n4/2021-11-vader/blob/429970427b4dc65e37808d7116b9de27e395ce0c/contracts/governance/GovernorAlpha.sol#L545

Handle

ye0lde

Vulnerability details

Impact

Open TODOs can point to architecture or programming issues that still need to be resolved.

Proof of Concept

The TODOs are here: https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/math/VaderMath.sol#L80 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/pool/BasePool.sol#L163 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/pool/VaderPool.sol#L85 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/pool/VaderPool.sol#L93 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/router/VaderRouter.sol#L303 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex/utils/GasThrottle.sol#L11

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/pool/VaderPoolV2.sol#L157 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/pool/VaderPoolV2.sol#L209 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/pool/VaderPoolV2.sol#L265 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/dex-v2/pool/VaderPoolV2.sol#L400

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/tokens/USDV.sol#L38

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/contracts/twap/TwapOracle.sol#L125

https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/repo/vader-bond/contracts/VaderBond.sol#L299 https://github.com/code-423n4/2021-11-vader/blob/3a43059e33d549f03b021d6b417b7eeba66cf62e/repo/vader-bond/contracts/VaderBond.sol#L336

Tools Used

VS Code

Recommended Mitigation Steps

Consider resolving the TODOs before deploying.