code-423n4 / 2022-09-canto-findings

0 stars 0 forks source link

Hackers can deploy token with respective name as the stable one to impersonate the stable token #24

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-canto/blob/65fbb8b9de22cf8f8f3d742b38b4be41ee35c468/src/Swap/BaseV1-periphery.sol#L490-L508

Vulnerability details

Impact

Hackers can deploy tokens with respective names as the stable ones to impersonate the stable token. Then hackers can get profit from the malicious price oracle.

Proof of Concept

        string memory symbol = ctoken.symbol();
        if (compareStrings(symbol, "cCANTO")) {
            underlying = address(wcanto);
            return getPriceNote(address(wcanto), false);
        } else {
            underlying = address(ICErc20(address(ctoken)).underlying()); // We are getting the price for a CErc20 lending market
        }
        //set price statically to 1 when the Comptroller is retrieving Price
        if (compareStrings(symbol, "cNOTE")) { // note in terms of note will always be 1 
            return 1e18; // Stable coins supported by the lending market are instantiated by governance and their price will always be 1 note
        } 
        else if (compareStrings(symbol, "cUSDT") && (msg.sender == Comptroller )) {
            uint decimals = erc20(underlying).decimals();
            return 1e18 * 1e18 / (10 ** decimals); //Scale Price as a mantissa to maintain precision in comptroller
        } 
        else if (compareStrings(symbol, "cUSDC") && (msg.sender == Comptroller)) {
            uint decimals = erc20(underlying).decimals();
            return 1e18 * 1e18 / (10 ** decimals); //Scale Price as a mantissa to maintain precision in comptroller
        }

If hackers or malicious admin deploy a non-stable token but has "cNOTE", "cUSDT", or "cUSDC" as symbols, these tokens will act as a stable token while in fact, it isn't.

Recommended Mitigation Steps

Use fixed address whitelisting instead for example if (address(ctoken) == cUSDC_address) ... where cUSDC_address is an immutable variable set on the constructor.

tkkwon1998 commented 2 years ago

The warden is saying that malicious parties could deploy a token with the same name to impersonate other tokens, but that these tokens will use the same pricing methodology as the token it is impersonating.

However, as this is an open EVM, anyone could deploy any token name with any underlying price method. I fail to see how this would be an issue, since everything is open source and users can see what they are doing.

0xean commented 2 years ago

What is the benefit the sponsor is trying to achieve by comparing a non unique string compared to a unique address?

While everything is open source, it does seem like this is a pretty bad design choice when compared to the alternative solutions.

nivasan1 commented 2 years ago

@0xean, the design choice presented here prevents from having multiple dependencies in the initialization of the oracle. Furthermore, the tokens that are referenced here must be supported by governance, so to override the actual prices would require a >51% voting power. As such, even if the exploit exists it is essentially impossible. As such, we do not believe that this is a high / med -risk issue.

0xean commented 2 years ago

Will downgrade to M as there are several external factors for this to become an issue.