EBTCToken is not compliant with ERC-20

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-badger/blob/f2f2e2cf9965a1020661d179af46cb49e993cb7e/packages/contracts/contracts/EBTCToken.sol#L134-L150

Vulnerability details

Impact

EBTCToken is defined as: "ERC20 EbtcToken, with permit approvals and extensible minting.".

However, there are some deviations from ERC-20 in its implementation, which implies, that EBTCToken is not compliant with ERC-20.

This may cause unexpected behavior due to being non compliant with ERC-20. Other protocols that integrate with contract may incorrectly assume that it's ERC-20 compliant - especially that documentation states that it's ERC-20. Any deviation from this standard will broke the composability and may lead to fund loss. While protocol's implements a contract and describes it as ERC-20, it should fully conform to ERC-20 standard.

During the previous Code4rena, lack of EIP compliance were evaluated as High/Medium:

• (Lack of EIP-4626 compliance) Vault.sol is not EIP-4626 compliant  2022-09-y2k-finance-findings#47 - has been evaluated as High
• (Lack of EIP-4524 compliance) HolographERC20 breaks composability by forcing usage of draft proposal EIP-4524 2022-10-holograph-findings#440 - has been evaluated as Medium
• (Lack of EIP-721 compliance) HolographERC721.safeTransferFrom not compliant with EIP-721 2022-10-holograph-findings#203 - has been evaluated as Medium

Proof of Concept

File: EBTCToken.sol

    function transferFrom(
        address sender,
        address recipient,
        uint256 amount
    ) external override returns (bool) {
        _requireValidRecipient(recipient);
        _transfer(sender, recipient, amount);

        uint256 cachedAllowance = _allowances[sender][msg.sender];
        if (cachedAllowance != type(uint256).max) {
            require(cachedAllowance >= amount, "ERC20: transfer amount exceeds allowance");
            unchecked {
                _approve(sender, msg.sender, cachedAllowance - amount);
            }
        }
        return true;
    }

Function transferFrom(), firstly performs the transfer: _transfer() at line 140, and then - it checks for allowances (lines 142-146).
This makes this token to be not compliant with ERC-20 standard.
Let's check for the OpenZeppelin transferFrom() implementation:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/94697be8a3f0dfcd95dfb13ffbd39b5973f5c65d/contracts/token/ERC20/ERC20.sol#L154-L159

   function transferFrom(address from, address to, uint256 value) public virtual returns (bool) {
       address spender = _msgSender();
       _spendAllowance(from, spender, value);
       _transfer(from, to, value);
       return true;
   }

As demonstrated above, _spendAllowance() is called before _transfer(). While in EBTCToken token implementation - we firstly perform _transfer(), and then, we check for the allowance.

Tools Used

Manual code review

Recommended Mitigation Steps

Check for allowances before performing ERC-20 transfer.

Assessed type

ERC20

[bytes032]

Invalid

[c4-pre-sort]

bytes032 marked the issue as insufficient quality report

[GalloDaSballo]

This statement is beyond exagerated

[c4-sponsor]

GalloDaSballo (sponsor) disputed

[c4-judge]

jhsagd76 marked the issue as unsatisfactory:
Invalid