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

3 stars 1 forks source link

Contracts are vulnerable to fee-on-transfer accounting-related issues #514

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 3 months ago

Lines of code

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

Vulnerability details

Impact

The function(s) below transfer funds from the caller to the receiver via transferFrom(), but do not ensure that the actual number of tokens received is the same as the input amount to the transfer. If the token is a fee-on-transfer token, the balance after the transfer will be smaller than expected, leading to accounting issues. Even if there are checks later, related to a secondary transfer, an attacker may be able to use latent funds (e.g. mistakenly sent by another user) in order to get a free credit. One way to address this problem is to measure the balance before and after the transfer, and use the difference as the amount, rather than the stated amount.

Path: ./src/managers/LockManager.sol

376:            token.transferFrom(_tokenOwner, address(this), _quantity);  // @audit-issue

376,

Recommendation

To mitigate potential vulnerabilities and ensure accurate accounting with fee-on-transfer tokens, modify your contract's token transfer logic to measure the recipient's balance before and after the transfer. Use this observed difference as the actual transferred amount for any further logic or calculations. For example:

function transferTokenAndPerformAction(address token, address from, address to, uint256 amount) public {
    uint256 balanceBefore = IERC20(token).balanceOf(to);

    // Perform the token transfer
    IERC20(token).transferFrom(from, to, amount);

    uint256 balanceAfter = IERC20(token).balanceOf(to);
    uint256 actualReceived = balanceAfter - balanceBefore;

    // Proceed with logic using actualReceived instead of the initial amount
    require(actualReceived >= minimumRequiredAmount, "Received amount is less than required");

    // Further logic here
}

Assessed type

ERC20

c4-judge commented 3 months ago

alex-ppg marked the issue as unsatisfactory: Out of scope