 |
Prepared by: Curiousapple, Independent Security Researcher Duration: 1 day, Aug 25, 2023 |
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 R savings module.
It's an ERC-4626 vault where users can deposit their R tokens and receive R token rewards on a fixed-rate basis.
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
Repository: r-savings-module
Branch: Master
Commit: 672d84fa40423b12da0bb7228673d8ec750c1209
Contracts: FixedIncomeERC4626Capped.sol
| Acronym | |
|---|---|
| M | Medium |
| Q | Quality |
| I | Informational |
| ID | Title | Status |
|---|---|---|
| M-01 | The update to totalAssetsStored following the asset transfer could lead to the vault distributing more R than initially intended |
Fixed |
| Q-01 | Unnecessary overriding of decimals() | Fixed |
| I-01 | A noticeable jump in totalAssets() caused by top-up could result in a minor asset loss due to rounding for new depositor | Ack |
| I-02 | Consequences of Pre-Top-Up Actions on Total Assets and Depositor Yields | Ack |
[M-01] The update to totalAssetsStored following the asset transfer could lead to the vault distributing more R than initially intended
- Impact: Medium
- Likelihood: High
As you can see here:
function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal override {
ERC4626._deposit(caller, receiver, assets, shares);
_setTotalAssetsStored(totalAssets() + assets);
}
function _deposit(address caller, address receiver, uint256 assets, uint256 shares) internal virtual {
// If _asset is ERC777, `transferFrom` can trigger a reentrancy BEFORE the transfer happens through the
// `tokensToSend` hook. On the other hand, the `tokenReceived` hook, that is triggered after the transfer,
// calls the vault, which is assumed not malicious.
//
// Conclusion: we need to do the transfer before we mint so that any reentrancy would happen before the
// assets are transferred and before the shares are minted, which is a valid state.
// slither-disable-next-line reentrancy-no-eth
SafeERC20.safeTransferFrom(_asset, caller, address(this), assets);
_mint(receiver, shares);
emit Deposit(caller, receiver, assets, shares);
}The R savings module updates the totalAssetsStored after the underlying asset transfer and the minting of shares.
This becomes problematic when total assets are treated as equivalent to the balance, in case where the calculated total assets exceed what the contract has to offer.
function totalAssets() public view virtual override returns (uint256) {
---------
uint256 totalAssets_ = ((issuanceRate_ * vestingTimePassed * totalAssetsStored_) / (1e18)) + totalAssetsStored_;
/// If the acrrued interest exceeds the contract balance, hard-stop interest accrual.
return Math.min(totalAssets_, IERC20(asset()).balanceOf(address(this)));
}Due to the sequence of events where the _setTotalAssetsStored(totalAssets() + assets) state update takes place, the newly transferred assets would have already been factored into the balance calculation.
This results in the manual addition of + assets becoming redundant, which leads to the inflation of totalAssetsStored.
Two distinct scenarios of inflation exist for totalAssetsStored:
Case 1:
When the new deposit amount is MORE than the accumulated but yet unconsidered interest, the unrealized interest of previous shareholders gets included in totalAssetsStored, even if the contract lacks sufficient funds for it.
For instance, if Alice's initial deposit was 100e18 with a 5% interest rate, and the contract has only 105e18, then after 2 years, when Bob deposits 50e18:
_setTotalAssetsStored(totalAssets() + assets)
_totalAssets() = min(balance, calculated)
The output of totalAssets() considers Bob's deposit in the balance, while the 'calculated' value only accounts for Alice's deposit and interest.
This results in mistakenly treating Bob's deposited tokens as rewards for Alice and returning the calculated value.
Subsequently, Bob's balance is manually added.
So, the contract includes Alice's unrealized interest in totalAssetsStored, even though there were insufficient funds.
// POC
function testDoubleAssetsCountCase1() public {
/// 5% APR
uint256 issuanceRate5APR = 1585489599;
uint256 aliceDeposit = 100e18;
vm.startPrank(ALICE);
rSavings.deposit(1, ALICE);
/// Top up rewards
asset.transfer(address(rSavings), aliceDeposit * 0.05e18 / 1e18); // fund enough for 1 year
rSavings.updateEmissionParams(365 days * 5, issuanceRate5APR); // 5% APR for 5 years
rSavings.deposit(aliceDeposit - 1, ALICE);
skip(365 days * 2); // 2 years
console.log("totalAssets=>balance", rSavings.totalAssets());
rSavings.deposit(50e18, BOB);
console.log("totalAssets=>balance", rSavings.totalAssets());
// Incorrect Alice's interest addition for 2nd year
console.log("totalAssetsStored", rSavings.totalAssetsStored());
}
Case 2:
In scenarios where the new deposit amount is LESS than the accumulated but unconsidered interest, the user deposit done just before top up of rewards would be counted twice.
In this case, the output of totalAssets() is straightforward – it's the OldBalance + New deposit, to which +New deposit is once again added.
What's common in both cases is the inflation of totalAssetsStored.
// POC
function testDoubleAssetsCountCase2CompletePOC() public {
/*//////////////////////////////////////////////////////////
Setup
//////////////////////////////////////////////////////////////*/
/// 5% APR
uint256 issuanceRate5APR = 1585489599;
uint256 aliceDeposit = 100e18;
vm.startPrank(ALICE);
rSavings.deposit(1, ALICE);
/// Top up rewards
asset.transfer(address(rSavings), aliceDeposit * 0.05e18 / 1e18); // fund enough for 1 year
rSavings.updateEmissionParams(365 days * 5, issuanceRate5APR); // 5% APR for 5 years
rSavings.deposit(aliceDeposit - 1, ALICE);
skip(365 days * 2); // 2 years
// balance of contract (105e18) is less than the total assets calculated,
// since contract was funded for 1 year only, and 2 years have passed
// hence, total assets returns balance here
console.log("Before Deposit: totalAssets=>balance", rSavings.totalAssets());
// Output: 105000000000000000000
/*//////////////////////////////////////////////////////////
Deposit Prior to Topup
//////////////////////////////////////////////////////////////*/
rSavings.deposit(3e18, BOB);
// Total Assets keep returning balance which is 108e18 in this case, (105e18 + 3e18)
console.log("After Deposit: totalAssets=>balance", rSavings.totalAssets());
// However this totalAssetsStored update is wrong since its consider's bob deposit's twice
uint256 totalAssetsStoredAfterDeposit = rSavings.totalAssetsStored();
console.log("After Deposit: totalAssetsStored", rSavings.totalAssetsStored());
// failing assertion
assertEq(totalAssetsStoredAfterDeposit, rSavings.totalAssets());
/*//////////////////////////////////////////////////////////
Topup
//////////////////////////////////////////////////////////////*/
// totalAssetsStored < balance
// The difference, amount of last user deposit before topup (3e18), would then be distributed to current share holders as interest.
// Bob would get instant return $$$, incentivising him to deposit, before topup.
}Now you are not wrong if you state that as long as the calculated total assets remain higher than the contract's balance, the contract's functionality will continue to operate smoothly, even if totalAssetsStored is no longer accurate.
Nonetheless, this assertion becomes invalid following a top-up of R from Raft.
This action effectively leads to totalAssetsCalculated becoming less than the balance, and consequently, it's reintroduced into the calculation.
Considering the inflation of totalAssetsStored, the difference between the stored and intended amounts would be distributed among existing shareholders.
Furthermore, the depositor just before the Raft top-up would receive an immediate return.
Consider doing the state update for totalAssetsStored prior to assets transfer.
Fixed By https://github.com/raft-fi/r-savings-module/pull/8
The default decimals() of ERC4626 should produce the same result as the one defined within FixedIncomeERC4626Capped.
Hence, explicit overriding of decimals in FixedIncomeERC4626Capped is not required.
Consider removing the redundant override for decimals.
Fixed By https://github.com/raft-fi/r-savings-module/pull/7
[I-01] A noticeable jump in totalAssets() caused by top-up could result in a minor asset loss due to rounding for new depositor
Whenever a top-up occurs after the TotalAssetsCalculated > Balance condition is met, an immediate increase in total assets takes place.
While this post-top-up action doesn't particularly pose problems for existing depositors, it could lead to unexpected outcomes for new depositors.
For instance, new depositors might receive fewer shares than they initially anticipated.
Consider the following scenario:
TotalSupply: 100, TotalAssets: 200 (Calculated: 400, Balance: 200)
Let's say Alice, a user looking to deposit, visits the UI and observes that she will receive 0.5 shares for each token she deposits.
As a result, she submits a deposit transaction.
However, the top-up of 600 is finalized before her deposit is processed:
TotalSupply: 100, TotalAssets: 400
In this adjusted state, Alice ends up receiving only 0.25 shares for each token deposit.
The total value of her deposit remains unchanged, but the number of shares she obtains is modified.
In extremely rare cases, particularly when dealing with small deposits and significant fluctuations, a depositor might even receive no shares at all.
If there is a considerable jump, consider blocking deposits through UI for the top-up period.
Acknowledged
Whenever there is a top-up after the TotalAssetsCalculated > Balance case, any action performed before the top-up will finalize the current balance as the total assets up to that point and will nullify all the yield that existing depositors could have received.
Consequently, if someone withdraws prior to this top-up, not only will they receive a lesser amount, but they will also decrease the yield for everyone else.
In theory, one could even argue that this opens the door to a griefing vector, as someone could make a dummy deposit just before the top-up to cancel out the yield of others.
--
There is no straightforward solution for retroactively distributing assets for the design of this vault.
Yuval (Raft) :
Ok maybe it's not really an issue, but it's something that should probably be documented.
In the same scenario, where TotalAssetsCalculated > Balance, I would think that the ideal behavior would be that a topup would only be distributed from the moment of topping up the contract, rather than retroactively
this can be achieved by doing any action that would result in a _setTotalAssetsStored invocation
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.