Submitted by 0xRajeev, also found by Sherlock, pauliax, shw, and JMukesh
ERC20 implementations are not always consistent. Some implementations of transferand transferFrom could return false on failure instead of reverting. It is safer to wrap such calls intorequire()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.
While the contract uses Uniswaps TransferHelper library function safeTransfer in other places for ERC20 tokens, or OpenZeppelins saferTransferFrom for ERC721 tokens (both of which call the tokens transfer/transferFrom functions and check return value for success and return data), it misses using TransferHelper.safeTransferFrom in this one case on L610 in timeLockERC20() when tokens are transferred from owner to the vault and instead directly uses the tokens transferFrom() call without checking for its return value.
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 and updates the timelockERC20Balances accounting accordingly. The timeUnlockERC20(), transferERC20() or delegatedTransferERC20() calls for that token will fail because the vault contract balance would have less tokens than accounted for in timelockERC20Balances because of the previously failed (but ignored) transferFrom() call.
Recommend replacing use of
with
This will revert on transfer failure for e.g. if msg.sender does not have a token balance >= amount.
xyz-ctrl (Visor) acknowledged:
ghoul-sol (Judge) commented:
ztcrypto (Visor) patched:
