Rhaydden - Insufficient checks for Unlocked tokens and Denial of Service Attack

[sherlock-admin2]

Rhaydden

medium

Insufficient checks for Unlocked tokens and Denial of Service Attack

Summary

The ERC20LockableUpgradeable.sol contract has 2 issues within its _beforeTokenTransfer function which compromises the functionality and security of token transfers. I'll try to go straight to the point.

Vulnerability Detail

1.) Unconsidered Locked Amount: The _beforeTokenTransfer function lacks consideration for the locked amount when checking if the sender has enough unlocked tokens. In the event an attacker locks all tokens of an account, the account is effectively denied the ability to send any tokens.

2.) Insufficient Check for Unlocked Tokens: The _beforeTokenTransfer function has a flawed check for unlocked tokens, relying solely on the balance minus the locked amount. If the locked amount equals the balance, the condition balanceOf(from) - _lockedAmount[from] >= amount will always evaluate to false for any non-zero transfer amount which could possibly prevent legitimate transfers.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-12-flatmoney/blob/main/flatcoin-v1/src/misc/ERC20LockableUpgradeable.sol#L45-L59

function _beforeTokenTransfer(address from, address to, uint256 amount) internal virtual override {
        // Make sure the sender has enough unlocked tokens.
        // Note: the below requirement is not needed when minting tokens in which case the `from` address is 0x0.
        if (from != address(0)) {
            require(
                balanceOf(from) - _lockedAmount[from] >= amount,
                "ERC20LockableUpgradeable: insufficient unlocked balance"
            );
        }

        super._beforeTokenTransfer(from, to, amount);
    }

    uint256[49] private __gap;
}

Tool used

Manual Review

Recommendation

For the first issue, The _lock and _unlock functions should have access control to prevent unauthorized access. This could involve using the onlyOwner or onlyAuthorized modifiers to restrict who can call these functions. Alternatively, users could be allowed to reclaim their locked tokens under certain conditions, such as after a certain period of time.

For the second issue, I'd suggest the balance check logic within _beforeTokenTransfer should consider both the balance and locked amount, ensuring transfers are allowed even when the locked amount equals the balance.

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[nevillehuang]

Invalid,

1. If you lock tokens it make sense you cannot transfer
2. If locked amount equals balance of user, than they have no tokens to transfer, check is correct