state variables are updated without the user actually making payment
instances where are funds getting stuck and some players will not be able to withdraw sometimes
Per the audit documentation tokens that do not revert on failure are in the scope of this audit.
Proof of Concept
Some ERC20 Tokens do not revert on failure of the transfer function, but return a bool value instead. Some do not return any value. Therefore it is required to check if a value was returned, and if true, which value it is. This is not done on some places in these contracts.
When players lock their tokens with lock(...) function the player’s lockedToken.quantity is updated to include the amount tokens that the player entered.
However, if the the tokenContract is a token that does not revert on failure,
the players lockedToken.quantity will be over inflated after locking if their transfer to LockManager fails.
during unlocking, players can loose their funds if the transfer from the LockManager fails.
File: LockManager.sol
373: // Transfer erc tokens
374: if (_tokenContract != address(0)) { // normal ERC not ETH
375: IERC20 token = IERC20(_tokenContract);
376: @> token.transferFrom(_tokenOwner, address(this), _quantity);
377: }
...
379: lockedToken.remainder = remainder;
380: lockedToken.quantity += _quantity;
Alice locks 20_000e18 token A which is a token that does not revert on failure
20_000e18 transferred from Alice wallet to LockManager is not successful and balance of LockManager remains the same but lockedToken.quantity of Alice is increased by 20_000e18
Bobs transfer of 20_000e18 goes through successfully
Alice unlock(...) successfully
Bob tries to unlock(...) but there not enough tokens to settle him however his lockedToken.quantity decreased but the transfer was unsuccessful
Bob has lost his funds without any means of retrieval
Tools Used
Manual review
Recommended Mitigation Steps
Check for the existence and value of the returned data of the transferFrom(...) call. If there is a return value, it has to be true. This could be achieved by using Openzeppelins SafeERC20 library´s safeTransfer.
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L373-L380 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L421-L424
Vulnerability details
Impact
This can lead to
Per the audit documentation tokens that do not revert on failure are in the scope of this audit.
Proof of Concept
Some ERC20 Tokens do not revert on failure of the transfer function, but return a bool value instead. Some do not return any value. Therefore it is required to check if a value was returned, and if true, which value it is. This is not done on some places in these contracts.
When players lock their tokens with
lock(...)function the player’slockedToken.quantityis updated to include the amount tokens that the player entered.However, if the the
tokenContractis a token that does not revert on failure,lockedToken.quantitywill be over inflated after locking if their transfer toLockManagerfails.LockManagerfails.LockManageris not successful and balance ofLockManagerremains the same butlockedToken.quantityof Alice is increased by 20_000e18unlock(...)successfullyunlock(...)but there not enough tokens to settle him however hislockedToken.quantitydecreased but the transfer was unsuccessfulTools Used
Manual review
Recommended Mitigation Steps
transferFrom(...)call. If there is a return value, it has to be true. This could be achieved by using Openzeppelins SafeERC20 library´ssafeTransfer.Assessed type
Token-Transfer