Unclaimed yield of the user will be lost.

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-02-spectra/blob/383202d0b84985122fe1ba53cfbbb68f18ba3986/src/tokens/PrincipalToken.sol#L340-L366

Vulnerability details

Bug description

Yield in the protocol is determined by the ibtRate. An increase in ibtRate results in a positive yield for users. updateYield()

function updateYield(address _user) public override returns (uint256 updatedUserYieldInIBT) {
        (uint256 _ptRate, uint256 _ibtRate) = _updatePTandIBTRates();

        uint256 _oldIBTRateUser = ibtRateOfUser[_user];
        if (_oldIBTRateUser != _ibtRate) {
            ibtRateOfUser[_user] = _ibtRate;
        }
        uint256 _oldPTRateUser = ptRateOfUser[_user];
        if (_oldPTRateUser != _ptRate) {
            ptRateOfUser[_user] = _ptRate;
        }

        // Check for skipping yield update when the user deposits for the first time or rates decreased to 0.
        if (_oldIBTRateUser != 0) {
            updatedUserYieldInIBT = PrincipalTokenUtil._computeYield(
                _user,
                yieldOfUserInIBT[_user],
                _oldIBTRateUser,
                _ibtRate,
                _oldPTRateUser,
                _ptRate,
                yt
            );
            yieldOfUserInIBT[_user] = updatedUserYieldInIBT;
            emit YieldUpdated(_user, updatedUserYieldInIBT);
        }
    }

The above function is responsible for updating the yield for a user. It does so by calling _computeYield() of the PrincipalTokenUtil library. Assume a scenario where there are two users, each holding 100 PT/YT tokens after a deposit of 100 tokens of underlying. Let's say ibtRate increases by 50%, now userA claims his yield, but userB does not. After that ibtRate decreases by 33% to its original value and both users claim their yield. This will result in userA receiving 49 underlying tokens of yield, while userB will receive only 33 underlying tokens, which results in 16 tokens of yield lost for userB. While userA got rewarded for a positive increase in ibtRate, userB did not.

Important Actual yield of userA is 16 tokens and the actual yield of userB is 0, since 33 tokens are the compensation for the negative decrease of 33%.

Impact

Loss of yield for users.

Proof of Concept

Please add this POC to PrincipalToken3.t.sol and run it with forge test --match-test 'testLostYield' -vv.

function testLostYield() public {
// Current IBTRate = 1e27
// Current PTRate = 1e27
address user1 = makeAddr("user1");
vm.prank(user1);
underlying.approve(address(principalToken), type(uint256).max);
address user2 = makeAddr("user2");
vm.prank(user2);
underlying.approve(address(principalToken), type(uint256).max);
// Mint both users 100 USDC initially
underlying.mint(user1, 100e18);
underlying.mint(user2, 100e18);

    // Deposit 100 USDC for both users
    vm.prank(user1);
    principalToken.deposit(100e18, user1);
    vm.prank(user2);
    principalToken.deposit(100e18, user2);
    // Both users now hold 100 PT and 100 YT tokens
    assertEq(principalToken.balanceOf(user1), 100e18);
    assertEq(principalToken.balanceOf(user2), 100e18);
    assertEq(yt.balanceOf(user1), 100e18);
    assertEq(yt.balanceOf(user2), 100e18);

    // IBTRate increases by 50% to 1.5 and user1 claims his yield, but user2 does not claim his yield after that increase
    _changeRate(50, true);
    vm.prank(user1);
    principalToken.claimYield(user1);

    // IBTRate decreases by 33% to the original value and user2 claims his yield
    _changeRate(33, false);
    vm.prank(user2);
    principalToken.claimYield(user2);

    vm.prank(user1);
    principalToken.claimYield(user1);

    console2.log(underlying.balanceOf(user1));
    console2.log(underlying.balanceOf(user2));
}

The console output of the above test:

49999999999999999898 33499999999999999954

## Recommended Mitigation
To be fair, I don't have a ready-made solution on how to handle this case gracefully in the current version of the codebase. Potentially, after a decrease in rate has been noticed, you could store the last known `maxIBTRate` value on-chain, and then when a user tries to claim yield after a decrease, you could first claim yield from his previous rate to that `maxIBTRate` (given he did not claim that yield already) and only then claim yield for the decrease. Even though this must be given a little more thought, I feel like this would be a step in the right direction.

## Assessed type

Other

[c4-pre-sort]

gzeon-c4 marked the issue as sufficient quality report

[c4-pre-sort]

gzeon-c4 marked the issue as duplicate of #109

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[c4-judge]

JustDravee marked the issue as unsatisfactory: Invalid

[kazantseff]

Hey @JustDravee, I think that this issue is different from #109. This submission describes a problem where yield is not automatically claimed when rates decrease. Could you please take another look?

[blckhv]

Hey @kazantseff, This vulnerability depends on the participants' timing and it is their own responsibility to maximize their profit by claiming when the ibt rate increases and not when it decreases. No one gives you 100% that using the protocol you will collect maximum yield.

[kazantseff]

@AydoanB Fair enough, it makes sense what you are saying.