 |
Raft FinanceInterest Rate VaultsPrepared by: Curiousapple, Independent Security Researcher Duration: 3 days, Sep 13, 2023 |
About
Raft Finance
In their own words, "Raft is a governance-minimized, decentralized protocol that allows people to generate R (a USD stablecoin) by depositing capital-efficient collateral."
This audit is for their new addition of Interest Rate Vaults.
This update is divided into two parts: Interest Debt Tokens and the Interest Position Manager.
Interest Debt Tokens allow Raft to apply a constant interest rate to debt positions, moving away from the previous implementation of the borrowing fee.
The Interest Position Manager is an update to the position manager designed to support the Interest Debt Tokens.
In Raft Team's own words,
What's being achieved and how its implemented:
1. We want to have interest rate borrowing
2. We mint R same way like we do with PSM
3. Simple interest rate based debt token is added to handle this
4. Interest is calculated the same way like in R Savings
Curiousapple 🦇
Abhishek Vispute, known online as 'Curiousapple', is an independent smart contract security researcher.
Previously, he served as a lead smart contract auditor at Macro and is currently working independently.
His auditing experience covers diverse set of protocols, including DeFi, Bridges, NFTs, DAOs, and Games, in all of which he has discovered severe bugs.
You can find his previous work and contact here
Scope
Pull Request: Add interest rate vaults
Main Changes:
- contracts/InterestRates/InterestRateDebtToken.sol
- contracts/InterestRates/InterestRatePositionManager.sol
- contracts/common/ERC20Capped.sol
- contracts/common/ERC20Minter.sol
Summary of Findings
| Acronym | |
|---|---|
| M | Medium |
| L | Low |
| Q | Quality |
| G | Gas |
| I | Informational |
| ID | Title | Status |
|---|---|---|
| M-01 | Newer Interest Rate would be considered for the past as well, increasing the debt of borrowers unfairly | Fixed |
| L-01 | Cap on debt tokens doesn't consider the interest accrued | Fixed |
| Q-01 | Incorrect Comment inside ConstantPriceFeed | Fixed |
| G-01 | Redundant actions in setIndex() |
Fixed |
| I-01 | Lack of tests for complex scenarios | Ack |
Detailed Findings
[M-01] Newer Interest Rate would be considered for the past as well, increasing the debt of previous borrowers unfairly.
- Impact: Medium
- Likelihood: Medium
function setIndexIncreasePerSecond(uint256 indexIncreasePerSecond_) public onlyOwner {
indexIncreasePerSecond = indexIncreasePerSecond_;
emit IndexIncreasePerSecondSet(indexIncreasePerSecond_);
}Since setIndexIncreasePerSecond doesn't call updateIndexAndPayFees() before, the newer interest would be considered for the duration of the current - previous index update as well.
Recommendation
Consider calling updateIndexAndPayFees before.
Doing so would apply the previous interest rate until now and would consider the new one moving forward.
Status
[L-01] Cap on debt tokens doesn't consider the interest accrued.
- Impact: Low
- Likelihood: Medium
Earlier, the cap was only enforced on ERC20.totalSupply(), and interest accrued was ignored.
function _mint(address account, uint256 amount) internal virtual override {
if (ERC20.totalSupply() + amount > cap) {
revert ERC20ExceededCap();
}
super._mint(account, amount);
}We had a small discussion here, regarding how debt cap should behave, and it was found that it should consider the indexable total supply instead.
Recommendation
Consider using the totalSupply from ERC20Indexable.
Status
[Q-01] Incorrect Comment inside ConstantPriceFeed
ConstantPriceFeed.sol, L10
/// @dev Price oracle to be used for the peg stability module to mint R.
/// Returns a constant price of 2 USD per token with 0 deviation.
contract ConstantPriceFeed is IPriceFeed, Lock {
It should be "Returns a constant price of 1 USD per token" instead.
Recommendation
Consider correcting the comment.
Status
[G-01] Redundant actions in setIndex()
function setIndex(uint256 backingAmount) public virtual override {
_payFees(currentIndex());
storedIndexUpdatedAt = block.timestamp;
super.setIndex(backingAmount);
}setIndex() is only called inside liquidations, and liquidations would always close positions first (debt burn), making the actions of paying fees and updating the state of storedIndexUpdatedAt redundant.
Recommendation
Consider removing the redundant code.
Status
[I-01] Lack of tests for complex scenarios
Consider adding unit tests for positions opened from InterestPositionManager on top of PositionManager for the following scenarios.
- Liquidation
- ManagePosition (4 cases: open position, debt change, collateral change, close position)
- Redistribution
In all cases (1, 2, 3), consider checking if fee updates are done correctly and should be applied prior to total supply changes.
Try to have complex initial states, including 2 interest collaterals, with 3 users participating, 2 for one collateral type, and 1 for another.
Consider checking how debt balance behaves in intermediate positions, whether that balance considers accrued interest correctly or not.
For example, a debt balance should include interest even if the index is not updated from current to stored yet.
Status
The Raft Team has acknowledged this and is actively working on adding complex tests.
For example: raft-fi/contracts#459
Disclaimer
curiousapple's review is limited to identifying potential vulnerabilities in the code. It does not investigate security practices, operational security, or evaluate the code relative to a standard or specification.
curiousapple makes no warranties, either express or implied, regarding the code's merchantability, fitness for a particular purpose, or that it's free from defects.
curiousapple will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, procurement costs of substitute goods or services, or any claim by any other party.
curiousapple will not be liable for any consequential, incidental, special, indirect, or exemplary damages, even if it has been advised of the possibility of such damages.
This review does not constitute investment advice, is not an endorsement, and is not a guarantee as to the absolute security of the project.
By deploying or using the code, users agree to use the code at their own risk.
curiousapple is not responsible for the content or operation of any third-party websites or software linked or referenced in the review, and shall have no liability for the use of such.