Improper Adjustment of Lending Ledger Configuration

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2024-03-neobase/blob/main/src/LendingLedger.sol#L136-L149 https://github.com/code-423n4/2024-03-neobase/blob/main/src/LendingLedger.sol#L173-L185

Vulnerability details

Description

The LendingLedger utilizes an approximation system (to evaluate the time that has elapsed between two block numbers) which contains adjustable values. Additionally, it permits the governance to adjust the cantoPerBlock value of multiple epochs at any time.

The way these values are adjusted is insecure and thus can cause them to retroactively apply to markets that have not yet been updated.

In the case of the LendingLedger::setBlockTimeParameters, a notice exists within the LendingLedger::update_market function that warns that:

If this ever drifts significantly, the average block time and/or reference block time & number can be updated. However, update_market needs to be called for all markets beforehand.

I do not consider the warning sufficient, as the LendingLedger::setRewards function illustrates that the problem is not understood accurately.

Any market that was not updated on the exact same block that either rewards or the block time parameters are adjusted will have these adjustments retroactively applied, leading to over-estimations or under-estimations of the rewards that should be attributed to the market.

Impact

Reward measurements for markets that were not updated in the exact same block that rewards and/or block-time parameters were re-configured will result in over- or under-estimations, depending on the direction of these configurations.

Severity Rationalization

Administrator mistakes usually fall under QA / Analysis reports, however, in this circumstance, the mistake is not based on input but rather on the state of the contract. Additionally, there are cases whereby the change cannot be performed securely (i.e. if the number of markets introduced would reach the gas limit if all are attempted to be updated at the same block).

Based on the above, I believe this constitutes a valid operational vulnerability that stems from an improperly coded configuration procedure for both rewards and block time parameters.

While there is a warning for the LendingLedger::setBlockTimeParameters function, it is located in an entirely different function and is insufficient IMO. Even if considered sufficient, there is absolutely no warning or indication that the same restriction applies for the LendingLedger::setRewards function.

This submission may be split into two distinct ones if the reward-related and block-time-related impacts are considered distinct, however, I grouped them under the same submission as they pertain to the same operation (update of all markets) being absent albeit from two different code segments.

Proof of Concept

I have coded the following PoC in foundry for completeness. Please add the following segment after LendingLedger.t.sol::setupStateBeforeClaim:

function testInsecureRewardUpdate() public {
    setupStateBeforeClaim();

    // Based on `LendingLedger.t.sol::testClaimValidLenderOneBlock`, the reward of the `lender` should be `amountPerBlock - 1` at this time point
    vm.roll(BLOCK_EPOCH * 5 + 1);

    // We update the rewards of the epochs without updating the markets
    vm.prank(goverance);
    uint256 newRewardPerBlock = amountPerBlock * 2;
    ledger.setRewards(fromEpoch, toEpoch, newRewardPerBlock);

    // We claim the `lender` rewards, should be `amountPerBlock` based on `LendingLedger.t.sol::testClaimValidLenderOneBlock`
    uint256 balanceBefore = address(lender).balance;
    vm.prank(lender);
    vm.roll(BLOCK_EPOCH * 5 + 1);
    ledger.claim(lendingMarket);
    uint256 balanceAfter = address(lender).balance;

    // Assertion will fail
    assertEq(balanceAfter - balanceBefore, amountPerBlock - 1);
}

To execute the above test, I recommend the following CLI instruction:

forge test --match-test testInsecureRewardUpdate -vv 

Tools Used

Manual Review.

Recommended Mitigation Steps

I advise all markets added to a LendingLedger to be tracked and iterated whenever either of the submission's referenced functions is executed, ensuring that all markets are indeed updated in the same block.

As a gas-optimal alternative, the total markets of the LendingLedger could be tracked as a number. The aforementioned adjustment functions could then accept an input array that would contain all markets, permitting a for loop to iterate them, ensure they are distinct (i.e. in strictly ascending order), and ensure that the total number of input markets is equal to the total number of registered markets.

Alternatively and as a solution solely for the LendingLedger::setRewards function, the epochs that are mutated could be restricted to be future ones and thus never result in a retroactive application.

Assessed type

Governance

[c4-judge]

MarioPoneder marked the issue as primary issue

[MarioPoneder]

Referring to README:

Publicly Known Issues

Mistakes by Governance: We assume that all calls that are performed by the governance address are performed with the correct parameters.

However, it's not just about correct call parameters but also about correct call timing (reward-related and block-time-related impacts), therefore leaving this for sponsor review.
The current duplicate #19 is more affected by the README; will reconsider during judging.

[c4-sponsor]

OpenCoreCH (sponsor) confirmed

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report

[MarioPoneder]

Administrator mistakes usually fall under QA / Analysis reports, however, in this circumstance, the mistake is not based on input but rather on the state of the contract.

I agree with this assessment in the report.