Vader Protocol contest - jvaqa's results

Handle

jvaqa

Vulnerability details

Impact

Gas Optimization: in Vader.sol, Turn Variables _1m, maxSupply, and decimals, into Constants/Immutable variables

Currently, in Vader.sol, the variables "_1m", "maxSupply", and "decimals", are kept mutable, incurring an SLOAD call on every invocation. However, the variable "_1m" is only ever used to refer to the constant value of 1 million * weiDecimals, the variable maxSupply is only ever used to refer to 2 million * weiDecimals, and decimals is only ever used to refer to 10 ** 18. If you make these variables constant, the compiler will inline those values into every call to _1m, maxSupply, and decimals, avoiding all SLOAD's in any calls that use them.

Proof of Concept

In Vader.sol's constructor, the following lines are used: decimals = 18; _1m = 10**6 * 10 ** decimals; //1m maxSupply = 2 * _1m;

Recommended Mitigation Steps

If you would like to set the value in the constructor rather than simply hardcoding it, then use the keyword "immutable" to set it once and never use an SLOAD again. Or, you can simply hardcode the value in the variable's declaration, and then use the "constant" keyword.

Handle

jvaqa

Vulnerability details

Impact

Avoid Unnecessary Expensive SSTORE Calls In Vether.sol By Checking If _fee Is Non-Zero

SSTORE calls (writes to storage) are very expensive, especially for cold-storage slots (those that have not yet been accessed this transaction). We know that the SSTORE call to totalFees will be a cold storage call, since this is the only place in the whole contract that totalFees is used. Vether.sol makes two SSTORE calls in _transfer that are unnecessary when _fee is zero. It will be common for _fee to be zero, since Vether.sol implements an "excluded addresses" list (mapAddress_Excluded), where _fee is zero when either the sender or the recipient is on the excludedAddresses list. Currently, anyone can add themselves to the excludedAddresses list, but that is probably a mistake. Nevertheless, since it will probably at least include Uniswap, we should add a check for whether _fee is zero.

Proof of Concept

When _fee is zero, Vether._transfer() nevertheless makes these two unnecessary SSTORE calls:

_balances[address(this)] += _fee; totalFees += _fee;

Recommended Mitigation Steps

Change this:

_balances[address(this)] += _fee; totalFees += _fee;

To this:

if(_fee > 0){ _balances[address(this)] += _fee; totalFees += _fee; }

Handle

jvaqa

Vulnerability details

Proof of Concept

This check is unnecessary since Solidity 0.8.x prevents overflows: require(_balances[_to] + _value >= _balances[_to], 'Balance overflow');

Recommended Mitigation Steps

Remove the aforementioned line of code, since Vether.sol uses Solidity 0.8.x, so it will revert on an overflow anyways.

Handle

jvaqa

Vulnerability details

Impact

Gas Optimization: Vader.sol Unnecessary Conditional

You can remove this conditional entirely.

Recommended Mitigation Steps

In Vader.sol, change this:

if(emitting){ emitting = false; } else { emitting = true; }

To this:

emitting = !emitting;

Handle

jvaqa

Vulnerability details

Impact

Gas Optimization: Utils.sol Make An Unnecessary Multiplication And Division By An Identical Value

The value "(T1 * B1) / T1" is identical to the value "B1", so you can simplify the expression "B1 + (T1 * B1) / T1" to "B1 + B1".

Recommended Mitigation Steps

In Utils.sol, replace this:

uint _redemptionValue = B1 + (T1 * B1) / T1;

With this:

uint _redemptionValue = B1 + B1;

Handle

jvaqa

Vulnerability details

Impact

Gas Optimization: DAO.sol Unnecessary Multiple Return Statements

Recommended Mitigation Steps

In DAO.sol, replace this:

if(votes > consensus){ return true; } else { return false; }

With this:

return (votes > consensus)

Handle

jvaqa

Vulnerability details

Impact

The Calculation For nextEraTime Drifts, Causing Eras To Occur Further And Further Into The Future.

In Vader.sol, eras are intended to occur every 24 hours. This means that a correct implementation would add 24 hours to the end-time of the previous era to find the end-time of the next era. However, the current method for calculating the next era's end-time uses block.timestamp, rather than the previous era's end-time.

Proof of Concept

This line of code will cause a perpetual drift of era times, causing each era to actually be 24 hours plus the time between when the last era ended and when Vader._transfer() is next called.

Recommended Mitigation Steps

In Vader.sol, change this:

nextEraTime = block.timestamp + secondsPerEra;

to this:

nextEraTime = nextEraTime + secondsPerEra;