-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pr variable rate 2 #3
base: main
Are you sure you want to change the base?
Conversation
} | ||
// Calculate per second rate | ||
interestRate = interestRate / 365 days; | ||
rateParameters.accumulated *= (1e18 + interestRate).wpow( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the referenced AAVE documentation the formula that is utilized for calculating the APY is:
However the implementation in the VariableInterestRateOracle.sol:get()
variable-rate-audit-DecorativePineapple/src/oracles/VariableInterestRateOracle.sol
Lines 215 to 224 in 2853396
// Calculate per second rate | |
interestRate = interestRate / 365 days; | |
rateParameters.accumulated *= (1e18 + interestRate).wpow( | |
secondsSinceLastUpdate | |
); | |
rateParameters.accumulated /= 1e18; | |
rateParameters.lastUpdated = block.timestamp; | |
sources[base.b6()][kind.b6()] = rateParameters; | |
} |
rateParameters.accumulated *= (1e18 + interestRate).wpow(secondsSinceLastUpdate);`
Is the - 1
from the formula (- 1e18
due to the scaling) missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that Aave expects the "Actual APY" to be in the [0, 1] range, as in a 5% APY being reported as 0.05, while our interest rate oracle reports the same APY as being 1.05 of the original amount, which is also 5%.
It makes sense, because they are just reporting the APY, while we are accumulating it. If we would use 0.05 to represent a 5% increase, the accumulator would tend to zero.
That is what I think, but I would appreciate this being double checked by both @DecorativePineapple and @iamsahu, I'm less confident than I sound on what I'm saying.
|
||
/// @dev Repay all debt in a vault. | ||
/// The base tokens need to be already in the join, unaccounted for. The surplus base will be returned to msg.sender. | ||
function repay( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that borrowing and repaying in the same block is allowed in yield-vr
. AAVE implements a check here . Also Morpho implements a similar check. This can help mitigate potential state inconsistencies against flash - loan attacks. I guess that this wouldn't be a problem in a fixed-rate setup.
Please note that if you want to implement a similar check you cannot use block.number
if you want to deploy your contracts on Arbitrum
(since block.number
returns the L1 block.number
on Arbitrum), you have to create a wrapper that uses block.number
if the chainId
matches the chain of the Ethereum Mainnet and use Arbitrum ArbSys
arbBlockNumber function
if the chainId
matches the Abritrum
chainId
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting, could you point us to one of those potential state inconsistencies, in this protocol or any other?
function redeem(uint256 principalAmount, address receiver, address holder) external returns (uint256 underlyingAmount) { | ||
principalAmount = (principalAmount == 0) ? _balanceOf[address(this)] : principalAmount; | ||
_burn(holder, principalAmount); | ||
underlyingAmount = _convertToUnderlying(principalAmount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since wmul
rounds down, I think that a check require(underlyingAmount != 0)
has to be added.
Also, have you considered adding a check for the minimumUnderlyingAmount
?
No description provided.