code-423n4 / 2023-08-verwa-findings

8 stars 7 forks source link

A few map / nested map in LendingLedger.sol missing public modifier #144

Open itsmetechjay opened 1 year ago

itsmetechjay commented 1 year ago

Line of Code

https://github.com/OpenCoreCH/test-squad-verwa/blob/9b5b194075d5b84a8da9c9f8df072f34258e6520/src/LendingLedger.sol#L17

Impact

Impossible to query the crucial smart contract state in LendingLedger.sol for on-chain / off-chain service

Proof of Concept

    mapping(address => bool) public lendingMarketWhitelist;
    /// @dev Lending Market => Lender => Epoch => Balance
    mapping(address => mapping(address => mapping(uint256 => uint256))) lendingMarketBalances; // cNote balances of users within the lending markets, indexed by epoch
    /// @dev Lending Market => Lender => Epoch
    mapping(address => mapping(address => uint256)) lendingMarketBalancesEpoch; // Epoch when the last update happened
    /// @dev Lending Market => Epoch => Balance
    mapping(address => mapping(uint256 => uint256)) lendingMarketTotalBalance; // Total balance locked within the market, i.e. sum of lendingMarketBalances for all
    /// @dev Lending Market => Epoch
    mapping(address => uint256) lendingMarketTotalBalanceEpoch; // Epoch when the last update happened

    /// @dev Lending Market => Lender => Epoch
    mapping(address => mapping(address => uint256)) userClaimedEpoch; // Until which epoch a user has claimed for a particular market (exclusive this value)

    struct RewardInformation {
        bool set;
        uint248 amount;
    }
    mapping(uint256 => RewardInformation) rewardInformation;

This is a few crucial states in the LendingLedger.sol

the map / nested map

are all by default internal

without adding the public modifier

it is impossible to query onchain data to know some very common info for both onchain or offchain intergration

for example, a offchain or onchain service may interest in query

-what is the lending market balance and a lending balance of a user in a market? we cannot get it because the lendingMarketBalance and lendingMarketTotalBalance are missing public modifier

there is also no method to preview the reward for a epoch as well, I can imagien it is very natural for external service to query such info for display purpose

https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

In this case, the missed public modifer greatly impact the availability of the LendingLedger.sol contract

Tools Used

Manual Review

Recommended Mitigation Steps

Add public modifier to following:

and add a method to query summarized user info and the reward that are eligible to claim given a epoch range

itsmetechjay commented 1 year ago

As noted in the README for this audit:

This audit was preceded by a Code4rena Test Coverage competition, which integrates a swarm approach to smart contract unit test coverage.

While auditing was not the purpose of the testing phase, relevant and valuable findings reported during that phase will be considered. Auditors who identified vulnerabilities during the test coverage phase will be eligible for a share of the pot, with H/M findings identified reviewed and judged as solo findings.

As such, C4 staff have added the above finding that was submitted by ladboy233 on July 26, 2023 at 4:51PM CDT as part of the test coverage competition.

141345 commented 1 year ago

QA might be more appropriate.

OpenCoreCH commented 1 year ago

Pasting my comment from the testing contest here for reference:

Agree that making userClaimedEpoch and rewardsInformation public could be helpful for UX / DevEx, but for the other ones, I think making them public could be rather dangerous. lendingMarketBalances and lendingMarketTotalBalance are only set after checkpoints have been performed and if a consumer does not know / check this, he might get 0 back, but only because no checkpoint for this epoch was done yet. So I think a dedicated view function that reverts for non-checkpointed epochs might make more sense there.

I am also not sure if this really is a medium issue. Not exposing this information does not impact the functionality of our intended usage. But I'll leave that to the judge.

c4-sponsor commented 1 year ago

OpenCoreCH marked the issue as sponsor acknowledged

c4-sponsor commented 1 year ago

OpenCoreCH marked the issue as disagree with severity

c4-judge commented 1 year ago

alcueca changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

alcueca marked the issue as grade-a