ShadowForce - call data to enter or exit market is never generated When executing rebalancing in TreasuryAction

[sherlock-admin]

ShadowForce

high

call data to enter or exit market is never generated When executing rebalancing in TreasuryAction

Summary

call data to enter or exit market is never generated

Vulnerability Detail

the function getDepositCalldata never generates calldata for entering an exiting compounds markets, this can be observed in the snippet below

function getDepositCalldata(
        address from,
        address assetToken,
        address assetRateAdapter,
        uint256 rateAdapterPrecision,
        uint256 depositUnderlyingAmount,
        bool underlyingIsETH
    ) internal view returns (DepositData[] memory data) {
        address[] memory targets = new address[](1);
        bytes[] memory callData = new bytes[](1);
        uint256[] memory msgValue = new uint256[](1);

        targets[0] = assetToken;
        msgValue[0] = underlyingIsETH ? depositUnderlyingAmount : 0;
        callData[0] = abi.encodeWithSelector(
            underlyingIsETH ? CEtherInterface.mint.selector : CErc20Interface.mint.selector, 
            depositUnderlyingAmount
        );

        data = new DepositData[](1);
        data[0] = DepositData(targets, callData, msgValue, depositUnderlyingAmount, assetToken);
    }

since no calldata for entering and exiting the compound markets is ever generated, it is impossible to enter the market rendering the function useless. Users of the protocol will be unable to correctly enter markets because of this

entering markets is vital for supplying collateral as stated in the Compound docs
https://docs.compound.finance/v2/comptroller/

Enter into a list of markets - it is not an error to enter the same market more than once. In order to supply collateral or borrow in a market, it must be entered first.

Notional wants to deposit asset and mint cToken and supply assets to generate interest rate and return for the user and also to accrue COMP token reward as well, this is core functionality of the protocol.

This is the desired outcome but because markets are never entered the above statement is impossible

Users will miss out on return and accrural of COMP tokens. This is a direct loss of funds for the user.

Impact

Because entering a market is crucial for minting cToken, the lack of calldata for entering markets will cause users to not accrue interest and COMP tokens, this is a direct loss of funds for the user.

Code Snippet

https://github.com/notional-finance/contracts-v2/blob/b20a45c912785fab5f2b62992e5260f44dbae197/contracts/external/pCash/adapters/CompoundV2AssetAdapter.sol#L31-L52

Tool used

Manual Review

Recommendation

We recommend generating calldata to enter and exit markets

[Jiaren-tang]

Escalate for 10 USDC. this should be a valid issue if #28 is a valid issue

if any new cToken is added, failed to call addMarket would make the rebalance flow ineffiecent because the asset never start to accure COMP reward

according to the doc of https://docs.compound.finance/v2/comptroller/

Enter Markets
Enter into a list of markets - it is not an error to enter the same market more than once. In order to supply collateral or borrow in a market, it must be entered first.

failed to enter market means the added cToken failed to be supply as collateral and cannot be borrowed to accure COMP reward and interest

[sherlock-admin]

Escalate for 10 USDC. this should be a valid issue if #28 is a valid issue

if any new cToken is added, failed to call addMarket would make the rebalance flow ineffiecent because the asset never start to accure COMP reward

according to the doc of https://docs.compound.finance/v2/comptroller/

Enter Markets
Enter into a list of markets - it is not an error to enter the same market more than once. In order to supply collateral or borrow in a market, it must be entered first.

failed to enter market means the added cToken failed to be supply as collateral and cannot be borrowed to accure COMP reward and interest

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[0xleastwood]

https://github.com/notional-finance/contracts-v2/blob/b20a45c912785fab5f2b62992e5260f44dbae197/contracts/external/pCash/adapters/CompoundV2AssetAdapter.sol#L31-L52

How does calldata fail to get generated. It is clearly there? The below snippet will encode the right data with the correct function selector to mint cTokens.

callData[0] = abi.encodeWithSelector(
            underlyingIsETH ? CEtherInterface.mint.selector : CErc20Interface.mint.selector, 
            depositUnderlyingAmount
        );

While, CEtherInterface.mint() does not actually take in any parameters, excess calldata when calling a function will be safely ignored, although it could be cleaned up a bit better.

[Jiaren-tang]

https://github.com/notional-finance/contracts-v2/blob/b20a45c912785fab5f2b62992e5260f44dbae197/contracts/external/pCash/adapters/CompoundV2AssetAdapter.sol#L31-L52

How does calldata fail to get generated. It is clearly there? The below snippet will encode the right data with the correct function selector to mint cTokens.

callData[0] = abi.encodeWithSelector(
            underlyingIsETH ? CEtherInterface.mint.selector : CErc20Interface.mint.selector, 
            depositUnderlyingAmount
        );

While, CEtherInterface.mint() does not actually take in any parameters, excess calldata when calling a function will be safely ignored, although it could be cleaned up a bit better.

Hi, I agree the correct mint for CEther and CERC20 token is generated, the the enterMarket is never generated given that the supported token can be USDT (in the future)

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Comptroller.sol#L121

    function enterMarkets(address[] memory cTokens) override public returns (uint[] memory) {
        uint len = cTokens.length;

        uint[] memory results = new uint[](len);
        for (uint i = 0; i < len; i++) {
            CToken cToken = CToken(cTokens[i]);

            results[i] = uint(addToMarketInternal(cToken, msg.sender));
        }

        return results;
    }

[0xleastwood]

https://github.com/notional-finance/contracts-v2/blob/b20a45c912785fab5f2b62992e5260f44dbae197/contracts/external/pCash/adapters/CompoundV2AssetAdapter.sol#L31-L52

How does calldata fail to get generated. It is clearly there? The below snippet will encode the right data with the correct function selector to mint cTokens.

callData[0] = abi.encodeWithSelector(
            underlyingIsETH ? CEtherInterface.mint.selector : CErc20Interface.mint.selector, 
            depositUnderlyingAmount
        );

While, CEtherInterface.mint() does not actually take in any parameters, excess calldata when calling a function will be safely ignored, although it could be cleaned up a bit better.

Hi, I agree the correct mint for CEther and CERC20 token is generated, the the enterMarket is never generated given that the supported token can be USDT (in the future)

https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/Comptroller.sol#L121

    function enterMarkets(address[] memory cTokens) override public returns (uint[] memory) {
        uint len = cTokens.length;

        uint[] memory results = new uint[](len);
        for (uint i = 0; i < len; i++) {
            CToken cToken = CToken(cTokens[i]);

            results[i] = uint(addToMarketInternal(cToken, msg.sender));
        }

        return results;
    }

You don't need to call enterMarkets() to earn interest on your cTokens. The exchange rate is automatically updated in each action within CEther.sol and CErc20.sol. This is used for borrowers and is not necessary to simply supply collateral to Compound.

[0xleastwood]

CToken.sol will call mintAllowed() and redeemAllowed() which does not care about this function.

[Trumpero]

Agree with the comments of Leastwood, this issue should be invalid since enterMarket is unnecessary to claim rewards of CToken.

[hrishibhat]

Result:
Invalid
Unique
Considering this a non-issue based on Lead Watson and Judge's comments as enterMarket is not required to claim rewards of CToken.

[Evert0x]

Escalations have been resolved successfully!

Escalation status:

• ShadowForce: rejected