code-423n4 / 2021-04-vader-findings

1 stars 0 forks source link

Vault Weight accounting is wrong for withdrawals #224

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

@cmichelio

Vulnerability details

Vulnerability Details

When depositing two different synths, their weight is added to the same mapMember_weight[_member] storage variable. When withdrawing the full amount of one synth with _processWithdraw(synth, member, basisPoints=10000 the full weight is decreased.

The second deposited synth is now essentially weightless.

Impact

Users that deposited more than one synth can not claim their fair share of rewards after a withdrawal.

Recommended Mitigation Steps

The weight should be indexed by the synth as well.

strictly-scarce commented 3 years ago

This is valid.

The weight should be reduced only as applied to a specific synth

There is no loss of funds, just less rewards for that member, disputing severity level.

Mervyn853 commented 3 years ago

Our decision matrix for severity:

0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.

Recommended: 2

dmvt commented 3 years ago

My viewpoint on this and the last several reward based high risk issues is that loss of rewards is loss of funds. High risk is appropriate.