crytic / slither

Static Analyzer for Solidity and Vyper
https://blog.trailofbits.com/2018/10/19/slither-a-solidity-static-analysis-framework/
GNU Affero General Public License v3.0
5.18k stars 953 forks source link

Function incorrectly identified as protected #1795

Open webthethird opened 1 year ago

webthethird commented 1 year ago

Describe the false alarm that Slither raise and how you know it's inaccurate:

Not a detector FP, but a FP from the Function.is_protected function.

In Compound's CErc20 contract. The mint(uint mintAmount) function is being incorrectly flagged as protected. I think it is because line 1489 of function.py sees msg.sender in the arguments being passed into the mintFresh(address minter, uint mintAmount) internal function on line 389 of CToken.sol. Yet there are no conditional statements in mintFresh which depend on the minter, so it should not be considered protected.

Interestingly, the redeem, redeemUnderlying and borrow functions are not flagged as protected, apparently because they all cast msg.sender to a payable address, i.e., borrowFresh(payable(msg.sender), borrowAmount) vs. mintFresh(msg.sender, mintAmount).

Frequency

Very Frequently

Code example to reproduce the issue:

CErc20.sol CToken.sol


contract CErc20 is CToken, CErc20Interface {
    ...
    function mint(uint mintAmount) override external returns (uint) {
        mintInternal(mintAmount);
        return NO_ERROR;
    }
    ...
}

abstract contract CToken is CTokenInterface, ExponentialNoError, TokenErrorReporter {
    ...
    function mintInternal(uint mintAmount) internal nonReentrant {
        accrueInterest();
        // mintFresh emits the actual Mint event if successful and logs on errors, so we don't need to
        mintFresh(msg.sender, mintAmount);
    }

    function mintFresh(address minter, uint mintAmount) internal {
        /* Fail if mint not allowed */
        uint allowed = comptroller.mintAllowed(address(this), minter, mintAmount);
        if (allowed != 0) {
            revert MintComptrollerRejection(allowed);
        }

        /* Verify market's block number equals current block number */
        if (accrualBlockNumber != getBlockNumber()) {
            revert MintFreshnessCheck();
        }

        Exp memory exchangeRate = Exp({mantissa: exchangeRateStoredInternal()});

        /////////////////////////
        // EFFECTS & INTERACTIONS
        // (No safe failures beyond this point)

        /*
         *  We call `doTransferIn` for the minter and the mintAmount.
         *  Note: The cToken must handle variations between ERC-20 and ETH underlying.
         *  `doTransferIn` reverts if anything goes wrong, since we can't be sure if
         *  side-effects occurred. The function returns the amount actually transferred,
         *  in case of a fee. On success, the cToken holds an additional `actualMintAmount`
         *  of cash.
         */
        uint actualMintAmount = doTransferIn(minter, mintAmount);

        /*
         * We get the current exchange rate and calculate the number of cTokens to be minted:
         *  mintTokens = actualMintAmount / exchangeRate
         */

        uint mintTokens = div_(actualMintAmount, exchangeRate);

        /*
         * We calculate the new total supply of cTokens and minter token balance, checking for overflow:
         *  totalSupplyNew = totalSupply + mintTokens
         *  accountTokensNew = accountTokens[minter] + mintTokens
         * And write them into storage
         */
        totalSupply = totalSupply + mintTokens;
        accountTokens[minter] = accountTokens[minter] + mintTokens;

        /* We emit a Mint event, and a Transfer event */
        emit Mint(minter, actualMintAmount, mintTokens);
        emit Transfer(address(this), minter, mintTokens);

        /* We call the defense hook */
        // unused function
        // comptroller.mintVerify(address(this), minter, actualMintAmount, mintTokens);
    }

### Version:

0.9.2

### Relevant log output:

_No response_
0xalpharush commented 1 year ago

What are you using is_protected to do?

webthethird commented 1 year ago

When auto-generating my differential fuzz testing contract, I'm using it to determine whether or not hevm.prank(msg.sender) should be used before calls to the contracts-under-test. For protected functions I don't want to, since the test contract is the admin.