I've put this as a medium issue as we're leaking value as users are stuck with assets which are likely to be worth much less as they are deprecated. It could also be low as it's not exploitable by outside parties and the loss isn't taken by the protocol but the user.
Impact
Potential for users to lose the right to convert VETH to VADER, being stuck with a deprecated token.
Proof of Concept
Should a user have a zero allowance of VETH on the converter, no VETH will be taken and no VADER will be paid out as L147 will set the amount to zero.
There is a minVader check on L153 to enforce a minimum output of VADER but should this be improperly set the transaction would succeed with the user receiving much less VADER than they expect.
Crucially, the merkle proof that was used in this transaction will be marked as invalid so the user will not be able to try again once they have set the proper allowance. Someone can then lose the opportunity to convert their VETH and are left with a worthless deprecated token if they are inattentive.
It seems like this is trying to handle the case where a user doesn't have the full amount of VETH as they are entitled to convert (by setting their allowance equal to their balance?). This is a pretty suboptimal way to go about this as it's extremely implicit so users are liable to make mistakes.
I'd recommend decoupling the merkle proof from conversion of VETH to VADER:
Change the merkle proof to set an amountEligibleToConvert value in storage for each user (which would be initially set to amount).
Allow a user to then convert VETH to VADER up to their amountEligibleToConvert value, subtracting the amount converted from this each time.
For gas efficiency we can use a sentinel value here to mark a user which has claimed their full quota already distinctly from someone who hasn't provided a merkle proof yet (to avoid having to track this separately in another storage slot)
These two steps could be chained in a single transaction to give a similar UX as currently but would also allow users to recover in the case of partial conversions.
Recommended Mitigation Steps
As above. I'd caution against just stating "The frontend will handle this correctly so this isn't an issue", there will be users who interact with the contract manually so it's important to make the interface safe where possible.
Handle
TomFrenchBlockchain
Vulnerability details
I've put this as a medium issue as we're leaking value as users are stuck with assets which are likely to be worth much less as they are deprecated. It could also be low as it's not exploitable by outside parties and the loss isn't taken by the protocol but the user.
Impact
Potential for users to lose the right to convert VETH to VADER, being stuck with a deprecated token.
Proof of Concept
Should a user have a zero allowance of VETH on the converter, no VETH will be taken and no VADER will be paid out as L147 will set the amount to zero.
https://github.com/code-423n4/2021-12-vader/blob/28d3405447f0c3353964ca755a42562840d151c5/contracts/tokens/converter/Converter.sol#L145-L150
There is a
minVadercheck on L153 to enforce a minimum output of VADER but should this be improperly set the transaction would succeed with the user receiving much less VADER than they expect.Crucially, the merkle proof that was used in this transaction will be marked as invalid so the user will not be able to try again once they have set the proper allowance. Someone can then lose the opportunity to convert their VETH and are left with a worthless deprecated token if they are inattentive.
It seems like this is trying to handle the case where a user doesn't have the full amount of VETH as they are entitled to convert (by setting their allowance equal to their balance?). This is a pretty suboptimal way to go about this as it's extremely implicit so users are liable to make mistakes.
I'd recommend decoupling the merkle proof from conversion of VETH to VADER:
amountEligibleToConvertvalue in storage for each user (which would be initially set toamount).amountEligibleToConvertvalue, subtracting the amount converted from this each time.For gas efficiency we can use a sentinel value here to mark a user which has claimed their full quota already distinctly from someone who hasn't provided a merkle proof yet (to avoid having to track this separately in another storage slot)
These two steps could be chained in a single transaction to give a similar UX as currently but would also allow users to recover in the case of partial conversions.
Recommended Mitigation Steps
As above. I'd caution against just stating "The frontend will handle this correctly so this isn't an issue", there will be users who interact with the contract manually so it's important to make the interface safe where possible.