Wild Chili Nuthatch - Unchecked repayLoanCallback Execution

[sherlock-admin4]

Wild Chili Nuthatch

High

Unchecked repayLoanCallback Execution

Summary
https://github.com/sherlock-audit/2024-11-teller-finance-update/blob/0c8535728f97d37a4052d2a25909d28db886a422/teller-protocol-v2-audit-2024/packages/contracts/contracts/LenderCommitmentForwarder/extensions/LenderCommitmentGroup/LenderCommitmentGroupShares.sol#L60-L71

Root Cause
The root cause of the state inconsistency issue in the _afterTokenTransfer() function is that it resets the poolSharesPreparedToWithdrawForLender[from] and poolSharesPreparedTimestamp[from] mappings whenever tokens are transferred. This behavior is triggered for all transfers, regardless of whether the transfer is related to share burning or not.

Detailed Explanation:
State Reset on Every Transfer: The function is designed to reset the prepared shares for the from address whenever a token transfer occurs. This includes transfers that are not related to share burns, which is problematic.

Unintended Consequences: A user might prepare their shares for burning (by calling prepareSharesForBurn()) and then transfer tokens for reasons unrelated to share burns (such as a normal token transfer). In such cases, the prepared shares and the timestamp for burning will be incorrectly reset, potentially locking the user out of burning their shares or leading to state inconsistencies.

Inconsistent Behavior: The poolSharesPreparedToWithdrawForLender and poolSharesPreparedTimestamp mappings are intended to track shares that are prepared for burning, but because the _afterTokenTransfer() function resets them on every transfer, it can cause confusion or unintended loss of state, especially in scenarios where a user makes transfers before burning.

PoC
To demonstrate the vulnerability, we will follow these steps:

Initial Setup:

The user prepares shares for burning using the prepareSharesForBurn() function.
The user has a certain amount of shares prepared to withdraw.
Transfer of Tokens:

The user transfers some tokens, triggering the _afterTokenTransfer() function.
Check Prepared Shares:

After the transfer, we check if the prepared shares and timestamp have been reset incorrectly.
PoC Steps:
User prepares shares for burning:

LenderCommitmentGroupShares sharesContract = LenderCommitmentGroupShares(address_of_contract));

// User prepares shares for burning
uint256 amountToPrepare = 1000;  // Example amount
sharesContract.prepareSharesForBurn(user_address, amountToPrepare);

At this point, the poolSharesPreparedToWithdrawForLender[user_address] will be set to 1000, and the timestamp will be recorded.

User transfers tokens:

// User transfers tokens to another address
uint256 amountToTransfer = 100;  // Example transfer amount
ERC20(token_address).transfer(other_user_address, amountToTransfer);

Check Prepared Shares:
After the transfer, we check the values of poolSharesPreparedToWithdrawForLender[user_address] and poolSharesPreparedTimestamp[user_address].

uint256 preparedShares = sharesContract.poolSharesPreparedToWithdrawForLender(user_address);
uint256 preparedTimestamp = sharesContract.poolSharesPreparedTimestamp(user_address);

// These values should remain intact, but they will be reset due to _afterTokenTransfer if not handled correctly
Expected Result (Without Fix):
The state values poolSharesPreparedToWithdrawForLender[user_address] and poolSharesPreparedTimestamp[user_address] will be reset to zero due to the _afterTokenTransfer() logic, even though the transfer was not related to share burning.

Impact:
The user will no longer be able to burn their shares as the prepared state was reset unexpectedly, causing an inconsistency in the contract's state.

Impact

The primary impact of the vulnerability in the _afterTokenTransfer() function is state inconsistency, which can lead to unintended consequences for users. Specifically, if a user prepares shares for burning and later transfers those shares, the prepared state is reset incorrectly. Here are the key consequences:

Inability to Burn Shares:

If a user transfers tokens, the contract will reset the prepared shares and the timestamp even though the transfer was not related to burning shares.
As a result, the user's ability to burn the prepared shares could be blocked because the system no longer tracks the prepared shares properly. The transfer of tokens should not affect the prepared withdrawal state, but in this case, it does.
Loss of Prepared State:

Users who prepare shares for burning may inadvertently lose their right to withdraw those shares after performing a normal transfer of tokens.
This leads to confusion and poor user experience, as users may not be aware that their shares were reset due to a non-relevant transfer.
Potential for Exploitation:

If the prepared shares are reset incorrectly, malicious actors could take advantage of this by transferring shares to manipulate the state and prevent other users from burning their shares. Although this may be difficult to exploit directly, it creates uncertainty and a potential attack surface for future issues.
Inconsistent Accounting:

The contract keeps track of shares that are prepared for burning using mappings like poolSharesPreparedToWithdrawForLender. Incorrect resets of these values may result in the wrong accounting, which could affect overall contract operations and the accuracy of the burn process.