 |
Prepared by: Curiousapple, Independent Security Researcher Duration: 2 days, Aug 31, 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 ChaiPSM Module.
It serves to swap reserve token (DAI) to and from R at 1:1 rate (minus the fee).
Two main user facing methods are BuyR() and BuyReserveToken()
- User sends "X" R
- Contract sends R to position manager back and gets "X" ChaiPSM back
- Contract burns "X" ChaiPSM.
- Contract deducts fee, withdraws "X-fee" amount of dai from chai and sends that to the user
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
Pull Request: PSM plus ChaiPSM
Contracts:
- contracts/PSM/BasePSM.sol
- contracts/PSM/ChaiPSM.sol
- contracts/PSM/ConstantPriceFeed.sol
- contracts/PSM/FixedFee.sol
- contracts/PSM/Lock.sol
- contracts/PSM/PSMSplitLiquidationCollateral.sol
No severe issues were found.
| Acronym | |
|---|---|
| Q | Quality |
| I | Informational |
| ID | Title | Status |
|---|---|---|
| Q-01 | Unnecessary Check of minReturn |
Ack |
| Q-02 | Incorrect Comment inside ConstantPriceFeed | Fixed |
| I-01 | withdrawReserve(), recoverTokens(), and setFees() could allow the owner/treasury to withdraw more reserve tokens than the yield alone |
Ack |
| I-02 | Impact of PSM on other position holders | Ack |
| I-03 | R will be soft-pegged to DAI | Ack |
The minReturn parameter is used in both the buyR() and buyReserveToken().
Its purpose is to allow users to specify the minimum amount of R or reserve token they should receive.
This is usually done to protect users against dynamic state changes, such as slippage.
However, in this case, the fees applied are fixed, so there won't be any change between transaction initiation and execution, unless Raft team updates the fee in between.
If fee updates are not regular and if big jumps are unlikely, minReturn parameter could be removed.
Consider removing the minReturn parameter from both buyR() and buyReserveToken() if fee updates are not regular and if big fee jumps are unlikely.
Ack
Mijovic (Raft) :
Regarding Q1, if we ever implement dynamic fees based on price oracles of reserve and R we might have a problem there. Also, we might build a fee for buying reserve based on the actual reserve in PSM. I know, we could just remove it and deploy new PSM with these features, but would try to do it without redeployment. LMK what do you think?
Curiousapple:
makes sense
/// @dev Price oracle to be used for peg stability module to mint R.
/// Returns constant price of 2 USD per token with 0 deviation.
contract ConstantPriceFeed is IPriceFeed, Lock {
/// Returns constant price of 2 USD per token with 0 deviation.
It should be "Returns constant price of 1 USD per token" instead.
Consider correcting the comment
[I-01] withdrawReserve(), recoverTokens(), and setFees() could allow the owner/treasury to withdraw more reserve tokens than the yield alone.
withdrawReserve:
The treasury or owner is expected to call withdrawReserve to collect the yield accrued on deposited DAI.
However, since there is no check on the amount passed, theoretically, withdrawal of more reserve tokens is also possible.
recoverTokens:
The same applies to recover tokens, as there is no check on the token passed, transfer of CHAI token from the PSM module is possible.
setFees:
Since there is no immutable check of the maximum on fees passed, fees could be set to any number < 100%.
There is no quick solution for the withdrawReserve() function.
Explicit on-chain accounting for yield accrued would be required to resolve it.
Solving the other two functions without solving withdrawReserve() is equivalent to solving none as well.
Hence, consider safeguarding the owner's address with all the best op-sec measures for now, until it is resolved.
Ack
Mijovic (Raft): We are aware if this. Originally I implemented it so that only yield can be taken out. With allowing removal of more we allow divesrsification of reserves by swapping it and deploying new PSM. This will be governance decision.
Please note that,
users who have deposited DAI in exchange for R would not be guaranteed to receive DAI back if someone else with different underlying collateral uses the PSM for their R → DAI conversion.
Since redemptions are currently disabled, users of (DAI → R) cannot redeem $1 worth of other collateral either.
Their only option would be to sell in the open market.
The reverse of the scenario mentioned above becomes possible once redemptions are enabled for other collateral types.
If someone with R derived from the PSM module redeems different collateral, the initial position owner who had created that position using different collateral cannot fully exit all of their R using the position manager alone.
They would need to exit some R using the position manager and remaining using the PSM module or open market.
Ack
Mijovic (Raft):
Redemptions are not going to be turned on ever.
Soon we will move all positions to interest based borrowing
Since there are no liquidations in the case of ChaiPSM collateral,
if DAI depegs, there won't be equivalent collateral backing for a given R.
Since redemptions are disabled forever moving forward, this won't impact other collateral positions for the position manager.
However, this would have an impact on the market value of R, since R, derived from PSM, now has a lower equivalent value.
People who had minted R using PSM, would look to sell their R in the open market.
Ack
Mijovic (Raft):
We are aware of this.
And we also plan to launch different PSMs for different reserves.
However, regarding soft peg to DAI, it is still true as most of our liquidity is paired with DAI anyway.
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.