Description

According to the NatSpec, the KittyPool::burnKittyCoin function should allow users to burn KittyCoins for other users and reduce debt.

It is an external function, so it can be called by anyone without restrictions. However, it is not entirely clear why it is declared as an external function, since it allows the caller of the function to reduce the debt of any user, but the KittyCoins are burned from the account of msg.sender, which is obviously a bug. It does also raise some important considerations regarding security and access control, as it is not ensured that the caller (msg.sender) is authorized to burn tokens on behalf of _onBehalfOfuser.
From an auditor's point of view, the function can be marked as internal and possibly used in KittyPool::purrgeBadPawsition to repay the user's debt (which is done right now in https://github.com/Cyfrin/2024-08-kitty-fi/blob/main/src/KittyPool.sol#L125-L126), or it should stay external, but be refactored so that the burning process of KittyCoins does not take place on the msg.sender, but on the address of a _onBehalfOf user (which obviously can also be the caller (msg.sender)).

Impact

The caller of the KittyPool::burnKittyCoin function could reduce the debt of any user, by adjusting the mapping values in kittyCoinMeownted[_onBehalfOf] -= _ameownt;, but would burn the KittyCoins from his own account as msg.sender. This would erroneously improve the collateral ratio of the _onBehalfOf user, which is checked in KittyPool::_hasEnoughMeowllateral, and would allow them to mint more KittyCoins than their actual collateral would support, thus inflating the protocol's token supply. On the other hand the caller of the function, in case the address of a _onBehalfOf user is not the caller (msg.sender) himself, would lose (or burn) his KittyCoins while not improving his own debt.

Tools Used

Manual review, vscode

Recommended Mitigation

It should be checked by the team behind KittyPool.sol what is the intended functionality of the KittyPool::burnKittyCoin function should be. We would recomend refactoring the function, so that the burning process and the modification of the kittyCoinMeownted mapping occur on the caller of the function (msg.sender). In this case it should be ensured that the burn operation and the mapping update are both applied to msg.sender rather than the _onBehalfOf address. Here's how you can modify the function: