// Eth must not remain in this contract at rest
payable(msg.sender).transfer(address(this).balance);
Impact
ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements or use safe wrapper functions implementing return value/data checks to handle these failures. For reference, see similar Medium-severity finding from Consensys Diligence Audit of Aave Protocol V2: https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom.
The impact can be that for an arbitrary ERC20 token, this transferFrom() call may return failure but the vault logic misses that, assumes it was successfully transferred into the vault
bareli
medium
uncheck output of "transfer"
Summary
no check on the output of transfer.
Vulnerability Detail
// Eth must not remain in this contract at rest payable(msg.sender).transfer(address(this).balance);
Impact
ERC20 implementations are not always consistent. Some implementations of transfer and transferFrom could return ‘false’ on failure instead of reverting. It is safer to wrap such calls into require() statements or use safe wrapper functions implementing return value/data checks to handle these failures. For reference, see similar Medium-severity finding from Consensys Diligence Audit of Aave Protocol V2: https://consensys.net/diligence/audits/2020/09/aave-protocol-v2/#unhandled-return-values-of-transfer-and-transferfrom. The impact can be that for an arbitrary ERC20 token, this transferFrom() call may return failure but the vault logic misses that, assumes it was successfully transferred into the vault
Code Snippet
https://github.com/sherlock-audit/2024-02-perennial-v2-3/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L162C8-L163C61
Tool used
Manual Review
Recommendation
use safetransfer instead of transfer.