The function _lock does not account for tokens that have a fee on transfer which results in the contract having less funds than perceived and the user having a bigger quantity deposited than they have which can result in the contract being drained.
Proof of Concept
Alice wants to lock 10 tokens which have a 10% fee on transfer. She calls lock() and reaches the point in the code where her tokens are taken:
if (_tokenContract != address(0)) { IERC20 token = IERC20(_tokenContract); token.transferFrom(_tokenOwner, address(this), _quantity); } ... lockedToken.quantity += _quantity;
The contract calls transferFrom with the value 10 because that is what Alice sent. But since the token has a 10% transfer fee the contract will receive 9 tokens but Alice will be registered as having 10 tokens.
After when Alice calls unlock to get her tokens back with the value 10, the contract will approve the call since Alice supposedly has 10 tokens. Then the function transfer will be called again with the value 10. Alice will receive 9 tokens because of the fee but the LockManager contract will have lost 1 token through the whole process.
Contract initial balance - 10 tokens
Contract balance after Alice called lock - 19 tokens
Contract balance after Alice called unlock - 9 tokens
Tools Used
Manual review
Recommended Mitigation Steps
Check the actual received tokens by saving balanceOf(address.this) before and after transferring the tokens and updating quantity with that value.
Lines of code
https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L311-L398
Vulnerability details
Impact
The function
_lockdoes not account for tokens that have a fee on transfer which results in the contract having less funds than perceived and the user having a biggerquantitydeposited than they have which can result in the contract being drained.Proof of Concept
Alice wants to lock 10 tokens which have a 10% fee on transfer. She calls
lock()and reaches the point in the code where her tokens are taken:if (_tokenContract != address(0)) { IERC20 token = IERC20(_tokenContract); token.transferFrom(_tokenOwner, address(this), _quantity); } ... lockedToken.quantity += _quantity;The contract callstransferFromwith the value 10 because that is what Alice sent. But since the token has a 10% transfer fee the contract will receive 9 tokens but Alice will be registered as having 10 tokens.After when Alice calls
unlockto get her tokens back with the value 10, the contract will approve the call since Alice supposedly has 10 tokens. Then the functiontransferwill be called again with the value 10. Alice will receive 9 tokens because of the fee but theLockManagercontract will have lost 1 token through the whole process.Contract initial balance - 10 tokens Contract balance after Alice called
lock- 19 tokens Contract balance after Alice calledunlock- 9 tokensTools Used
Manual review
Recommended Mitigation Steps
Check the actual received tokens by saving balanceOf(address.this) before and after transferring the tokens and updating
quantitywith that value.Assessed type
ERC20