search

code-423n4 / 2021-09-yaxis-findings

0 stars 0 forks source link

VaultHelper could validate that amount is greater than 0 #85

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

pauliax

Vulnerability details

Impact

functions depositVault, depositMultipleVault and withdrawVault in VaultHelper could require _amount > 0 to prevent useless transfers.

Recommended Mitigation Steps

Add require _amount > 0 statements to mentioned functions.

Haz077 commented 2 years ago

I think this should be labelled gas optimization.

uN2RVw5q commented 2 years ago

I think this should not be done in the smart contract, but rather as a front end check. Say, add this check in JavaScript before making the call. If we really want to, then this can be added as a comment in the documentation, for anyone integrating with the protocol.

Doing this in the smart contracts effectively means adding a if (amount > 0) { ...} check and skip the transfers for non-zero amounts. My rough calculation is that this check adds around 25 gas for every call to the function. The case of amount = 0 is an exceptional case, so adding this 25 gas overhead just to save gas in this rare case should be avoided.

I would recommend changing the severity to documentation.

GalloDaSballo commented 2 years ago

Agree with findings in theory and with sponsor mitigation, FE check is completely fine