The core problem lies in the fact that the function uses lastPayoutBal to calculate the melt amount, but then updates lastPayoutBal based on the current balance. This can lead to inconsistencies if the actual balance has changed between melt operations.
Code Snippet
function melt() public {
// ... (earlier parts of the function)
uint256 amount = payoutRatio.mulu_toUint(lastPayoutBal);
lastPayout += numPeriods;
lastPayoutBal = rToken.balanceOf(address(this)) - amount;
if (amount != 0) rToken.melt(amount);
}
The problem
amount is calculated based on lastPayoutBal.
lastPayoutBal is then updated using the current balance (rToken.balanceOf(address(this))).
If the actual balance has changed since the last melt (due to deposits or other operations), this new lastPayoutBal won't accurately reflect the starting balance for the next period.
Scenario
Initial lastPayoutBal: 1000 tokens
500 tokens are deposited to the contract
melt() is called:
amount is calculated based on 1000 tokens
lastPayoutBal is updated to (1500 - amount), which doesn't accurately represent the starting balance for the next period
Impact
The melting process may not accurately account for new deposits or withdrawals.
Over time, this could lead to either over-melting or under-melting compared to the intended behavior.
Potential fix
To address this, the function should either:
Use the current balance to calculate the melt amount, or
Keep track of deposits/withdrawals between melt operations.
A possible implementation of the first approach:
function melt() public {
// ... (earlier parts of the function)
uint256 currentBalance = rToken.balanceOf(address(this));
uint256 amount = payoutRatio.mulu_toUint(currentBalance);
lastPayout += numPeriods;
lastPayoutBal = currentBalance - amount;
if (amount != 0) rToken.melt(amount);
}
This change ensures that melting is always based on the current balance, accounting for any changes since the last melt operation.
Lines of code
https://github.com/code-423n4/2024-07-reserve/blob/3f133997e186465f4904553b0f8e86ecb7bbacbf/contracts/p1/Furnace.sol#L65
Vulnerability details
Vulnerability Details
The core problem lies in the fact that the function uses
lastPayoutBal
to calculate the melt amount, but then updateslastPayoutBal
based on the current balance. This can lead to inconsistencies if the actual balance has changed between melt operations.Code Snippet
The problem
amount
is calculated based onlastPayoutBal
.lastPayoutBal
is then updated using the current balance (rToken.balanceOf(address(this))
).lastPayoutBal
won't accurately reflect the starting balance for the next period.Scenario
lastPayoutBal
: 1000 tokensmelt()
is called:amount
is calculated based on 1000 tokenslastPayoutBal
is updated to(1500 - amount)
, which doesn't accurately represent the starting balance for the next periodImpact
Potential fix
To address this, the function should either:
A possible implementation of the first approach:
This change ensures that melting is always based on the current balance, accounting for any changes since the last melt operation.
Assessed type
Context