Reliance on ERC20Upgradable.decimals() will always return 18 despite actual token decimals

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/KnightingRound.sol#L148 https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/cf68a02973de4a8745dc457a82d48ce238419980/contracts/token/ERC20/ERC20Upgradeable.sol#L92-L94

Vulnerability details

Impact

A call to ERC20Upgradeable(_tokenIn).decimals() is used in the getAmountOut() function of KnightingRound.sol to determine how much citadel to provide to the user for the given amount of tokenIn. The issue with using ERC20Upgradeable.decimals() is that it always returns 18, despite the actual decimals of the token.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/token/ERC20/ERC20Upgradeable.sol#L92-L94

    function decimals() public view virtual override returns (uint8) {
        return 18;
    }

It's important to note that this function has the override keyword so it will override the possible decimals() function of the token contract.

If USDC is used as _tokenIn, the amount of citadel provided in exchange for the transferred USDC will be 10**-12 lower than it should be, via the following function.

    function getAmountOut(uint256 _tokenInAmount)
        public
        view
        returns (uint256 tokenOutAmount_)
    {
        tokenOutAmount_ =
            (_tokenInAmount * tokenOutPrice) /
            tokenInNormalizationValue;
    }

Proof of Concept

If USDC (6 decimals) is used, the amount of citadel returned will look as follows assuming a tokenOutPrice of 1 ether.

• 1 ether USDC (10**6 wei) sent to contract. Expected return value is 1 ether Citadel (10**18 wei).

tokenOutAmount_ = (10**6 USDC * 10**18) / 10**18 = 10**6 Citadel

Therefore, instead of receiving 10**18 Citadel, the user receives 10**6 Citadel.

Tools Used

Manual review.

Recommended Mitigation Steps

Instead of referencing ERC20Upgradeable(_tokenIn).decimals() to receive the token decimals, instead call IERC20Metadata.decimals() to ensure that the token complies with the decimals() call and DOES NOT override the function with a static value of 18

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/IERC20Metadata.sol

[GalloDaSballo]

The finding seems to come from a lack of understanding of Virtual Functions and Inheritance.

If a function is marked virtual it means we can override it. We can override the decimals so it doesn't matter what OZ says in the source code, you'd have to look at the child contract to confirm which decimals the token has.

Irrespective of that, if the contract where to return 18, and it had 18 decimals, there would be no vulnerability.

Have to fully disagree here