Allowance not updated correctly

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/ERC20.sol#L108 https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/ERC20.sol#L125

Vulnerability details

Impact

allowance doesn't get updated properly therefore an attacker contract can exploit the token contract by transferring some tokens and still spend the same amount of allowance. Here are the steps:

calling approve function from ERC20.sol with the spender address of test contract and uint256 value of 69664845694367813355683965970930918025071475522667657600662649263997801130727

and then calling the function transferFrom(address sender, address recipient, uint256 amount) from ERC20.sol with these inputs : sender: msg.sender recipient: 0xdeadbeef amount : 1 the allowance after should be equal to the allowance before minus the transfer amount which in this case is one. but in this case the result is not compliance to this assertion.

Proof of Concept

approve(test contract address,69664845694367813355683965970930918025071475522667657600662649263997801130727) from: https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/ERC20.sol#L108

test_ERC20external_spendAllowanceAfterTransfer function from test contract(can be an attacker contract) :

// TransferFrom should decrease allowance
function test_ERC20external_spendAllowanceAfterTransfer(
    address target,
    uint256 amount
) public {
    require(target != address(this) && target != address(0));
    require(target != msg.sender);
    uint256 balance_sender = token.balanceOf(msg.sender);
    uint256 current_allowance = token.allowance(msg.sender, address(this));
    require(balance_sender > 0 && current_allowance > balance_sender);
    uint256 transfer_value = (amount % balance_sender) + 1;
    emit LogUint256("transfer_value: ", transfer_value);
    emit LogUint256("current_allowance: ", current_allowance);
    bool r = token.transferFrom(msg.sender, target, transfer_value);//here it will call transferFrom function 
     //from ERC20.sol that I have addressed above. 
    assertWithMsg(r == true, "transferFrom failed");
    // Some implementations take an allowance of 2**256-1 as infinite, and therefore don't update
    if (current_allowance != type(uint256).max) {
        emit LogUint256("token.allowance :", token.allowance(msg.sender, address(this)));
        emit LogUint256(" current_allowance - transfer_value :",  current_allowance - transfer_value);
        assertEq(
            token.allowance(msg.sender, address(this)),
            current_allowance - transfer_value,
            "Allowance not updated correctly"
        );
    }
}

Call sequence: 1.approve(0xa329c0648769a73afac7f9381e08fb43dbea72,69664845694367813355683965970930918025071475522667657600662649263997801130727) 2.test_ERC20external_spendAllowanceAfterTransfer(0xdeadbeef,0)

Event sequence: Panic(1): Using assert., LogUint256(«transfer_value: », 1) from: 0xa329c0648769a73afac7f9381e08fb43dbea72, LogUint256(«current_allowance: », 69664845694367813355683965970930918025071475522667657600662649263997801130727) from: 0xa329c0648769a73afac7f9381e08fb43dbea72, Transfer(1) from: 0xb4c79dab8f259c7aee6e5b2aa729821864227e84, LogUint256(«token.allowance :», 69664845694367813355683965970930918025071475522667657600662649263997801130727) from: 0xa329c0648769a73afac7f9381e08fb43dbea72, LogUint256(« current_allowance - transfer_value:», 69664845694367813355683965970930918025071475522667657600662649263997801130726) from:0xa329c0648769a73afac7f9381e08fb43dbea72, AssertEqFail(«Invalid: 69664845694367813355683965970930918025071475522667657600662649263997801130727!=69664845694367813355683965970930918025071475522667657600662649263997801130726, reason: Allowance not updated correctly») from: 0xa329c0648769a73afac7f9381e08fb43dbea72

Tools Used

echidna

Recommended Mitigation Steps

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[0xA5DF]

PoC not clear enough (doesn't work when pasting into the tests file)

Anyways my guess is that this is due to the free allowance given to minter, this is not an issue

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid