sherlock-audit / 2024-08-sentiment-v2-judging

1 stars 0 forks source link

LonWof-Demon - Insufficient Balance Check and Allowance Underflow Vulnerability in Token Transfer Functions #492

Closed sherlock-admin2 closed 2 weeks ago

sherlock-admin2 commented 1 month ago

LonWof-Demon

Medium

Insufficient Balance Check and Allowance Underflow Vulnerability in Token Transfer Functions

Summary

The transfer and transferFrom functions in the provided Solidity code lack crucial checks and safeguards, potentially leading to issues like insufficient balance handling and allowance underflow.

Vulnerability Detail

  1. Lack of Balance Check in transfer Function:

    • Issue: The transfer function does not verify if the caller (msg.sender) has enough balance before attempting the transfer. If the balance is insufficient, this could lead to an underflow in the balance state, resulting in unintended behavior.
    • Potential Impact: This oversight can lead to incorrect balance updates, where balances might become negative (if using unchecked arithmetic), or other unintended consequences.
    //@audit if (balanceOf[sender][id] < amount) revert InsufficientBalance(sender, id);
  2. Allowance Underflow in transferFrom Function:

    • Issue: The transferFrom function does not check if the allowance is sufficient before subtracting the amount. This could lead to an allowance underflow if the amount exceeds the current allowance.
    • Potential Impact: If the allowance is not properly checked before the subtraction, it can lead to incorrect allowance values and potential security issues such as unauthorized transfers.
    //@audit require(allowance[sender][msg.sender][id] >= amount, "insufficient allowance");

    Additionally, if the allowance is equal to type(uint256).max, the code does not correctly handle the underflow situation. Although the intention might be to clear the allowance, not checking against underflow can be problematic.

    //@audit underflow of allowed amount is not enough to handle amounts

    Impact

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/lib/ERC6909.sol#L32 https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/main/protocol-v2/src/lib/ERC6909.sol#L42

function transfer(
    address receiver,
    uint256 id,
    uint256 amount
) public virtual returns (bool) {
    //@audit Missing balance check
    balanceOf[msg.sender][id] -= amount;
    balanceOf[receiver][id] += amount;
    emit Transfer(msg.sender, msg.sender, receiver, id, amount);
    return true;
}

function transferFrom(
    address sender,
    address receiver,
    uint256 id,
    uint256 amount
) public virtual returns (bool) {
    if (msg.sender != sender && !isOperator[sender][msg.sender]) {
        uint256 allowed = allowance[sender][msg.sender][id];
        //@audit Missing allowance check
        if (allowed != type(uint256).max) {
            allowance[sender][msg.sender][id] = allowed - amount;
        }
    }
    balanceOf[sender][id] -= amount;
    balanceOf[receiver][id] += amount;
    emit Transfer(msg.sender, sender, receiver, id, amount);
    return true;
}

Tool used

Manual Review

Recommendation

  1. For transfer Function:

    • Add a balance check before deducting the amount to ensure the sender has sufficient balance.
    require(balanceOf[msg.sender][id] >= amount, "Insufficient balance");
  2. For transferFrom Function:

    • Add a check to ensure the allowance is sufficient before performing the subtraction.
    require(allowance[sender][msg.sender][id] >= amount, "Insufficient allowance");
sherlock-admin2 commented 2 weeks ago

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

z3s commented:

it will revert