If for some reason the ERC20 transfer is temporarily failing, the user could call deposit() for free or if calling withdraw() the user would totally lose his allocation and funds. All the state variables would already have been updated at this stage, so he can’t call withdraw again. There is no way to withdraw these locked tokens.
Proof of Concept
At the last point of withdraw, the function is sending the funds to the user, and does not check the return value - whether it has succeeded:
Tools Used
vscode
Recommended Mitigation Steps
Add a check that verifies that the transfer has succeeded.
3xHarry
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L167 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L228 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L231 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L365
Vulnerability details
Impact
If for some reason the ERC20 transfer is temporarily failing, the user could call deposit() for free or if calling withdraw() the user would totally lose his allocation and funds. All the state variables would already have been updated at this stage, so he can’t call withdraw again. There is no way to withdraw these locked tokens.
Proof of Concept
At the last point of withdraw, the function is sending the funds to the user, and does not check the return value - whether it has succeeded:
Tools Used
vscode
Recommended Mitigation Steps
Add a check that verifies that the transfer has succeeded.