search

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

1 stars 0 forks source link

Transfer fee is burned on wrong accounts #220

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

@cmichelio

Vulnerability details

Vulnerability Details

The Vader._transfer function burns the transfer fee on msg.sender but this address might not be involved in the transfer at all due to transferFrom.

Impact

Smart contracts that simply relay transfers like aggregators have their Vader balance burned or the transaction fails because these accounts don't have any balance to burn, breaking the functionality.

Recommended Mitigation Steps

It should first increase the balance of recipient by the full amount and then burn the fee on the recipient.

strictly-scarce commented 3 years ago

For composabilty with the rest of the ecosystem, this should be addressed, although disagree with the severity, no funds are lost, just the aggregrator cannot transfer unless they first transfer to themselves, which most often do.

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