Lines of code

https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L374-L380 https://github.com/code-423n4/2024-05-munchables/blob/57dff486c3cd905f21b330c2157fe23da2a4807d/src/managers/LockManager.sol#L421-L424

Vulnerability details

According to readme ERC20 token behaviors in scope table this behavior is in scope.

Impact

After lock() fee-on-transfer token the amount of tokens passed as a parameter will be stored in the contract, but the actual amount of tokens received by the contract will be less.

        lockedToken.quantity += _quantity;

If this token is used frequently the actual balance of this token in the contract and sum of all users balances will be different. This lead to a situation when last user that will unlock() his tokens won't be able to do this. Because the contract balance will be less than he locked. As a result, user will lose his tokens.

Proof of Concept

Assume that fee-on-transfer token has 10% fee on transfer.

Alice locks 100 tokens in the contract. (90 tokens contract receives)
Bob locks 100 tokens in the contract. (90 tokens contract receives)
Alice unlocks 100 tokens. (contact sends 100 tokens to Alice - she receives 90 tokens)
- contract balance: 80 tokens
Bob tries to unlock his 100 tokens, but since contract balance is 80 tokens tx will revert.

Tools Used

manual review

Recommended Mitigation Steps

To avoid this behavior, lockedToken.quantity should be updated with the actual amount of tokens received by the contract.

        if (_tokenContract != address(0)) {
            IERC20 token = IERC20(_tokenContract);
+           uint256 balanceBefore = token.balanceOf(address(this));
            token.transferFrom(_tokenOwner, address(this), _quantity);
+           uint256 balanceAfter = token.balanceOf(address(this));
+           lockedToken.quantity += balanceAfter - balanceBefore;
+       } else {
+           lockedToken.quantity += _quantity;
+       }

        lockedToken.remainder = remainder;
-       lockedToken.quantity += _quantity;

Assessed type

Token-Transfer

code-423n4 / 2024-05-munchables-findings

Using Fee-On-Transfer Tokens will lead to user token loss. #515