 |
Prepared by: curiousapple, Independent Security Researcher Duration: 2 days, May 22 to May 23, 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."
Initially, this capital-efficient collateral was only wstETH. Then, Raft Team was looking to accept other tokens such as rETH and WETH as collateral, but they wanted to implement a cap on the total supply and maximum balance on the position.
The currently deployed contracts don't allow for this to be achieved, as they lack any consideration for supply and maximum balance per collateral token. Consequently, without altering the already deployed contracts, they developed a wrapped collateral token contract. This essentially acts as a wrapper around the underlying assets, enforcing limits on deposits.
In addition, new oracles were integrated to support the recent inclusions, and the existing ones were refactored.
This update also saw the liquidation protocol fee reduced to zero.
This audit includes the security review for these updates.
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
The scope of the audit included changes made to contracts from v1.0 tag to commit d5ca8febd9b7f33e1a3ad1eff1f2ae1d9dcd5b5f
Difference: raft-fi/contracts/compare/v1.0...d5ca8feb
The following contracts were in scope:
- BasePriceOracle.sol
- ChainlinkPriceOracle.sol
- ChainlinkPriceOracleRETH.sol
- ChainlinkPriceOracleWstETH.sol
- TellorPriceOracle.sol
- TellorPriceOracleWstETH.sol
- PositionManager.sol
- PositionManagerStETH.sol
- PositionManagerWETH.sol
- PositionManagerWrappedCollateralToken.sol
- SpiltLiquidationCollateral.sol
- WrappedCollateralToken.sol
| ID | Title | Status |
|---|---|---|
| H-01 | maxBalance Creates Various Issues for All Protocol Actions, Potentially Leading to Denial of Service on Existing Borrower's Position |
Fixed |
| Q-01 | SafeERC20 extension not used for _underlyingCollateralToken inside PositionManagerWrappedCollateralToken |
Fixed |
| Q-02 | Check Effects Interaction pattern not followed for managePositionETH() |
Fixed |
| Q-03 | Case of debtChange == type(uint256).max not accounted for in managePositionETH() of PositionManagerStETH |
Fixed |
| G-01 | Redundant check on collateralToRedeem |
Won't Do |
[H-01] maxBalance Creates Various Issues for All Protocol Actions, Potentially Leading to Denial of Service on Existing Borrower's Position
- Impact: High
- Likelihood: High
From the perspective of the wrapped token, the position manager is a individual only. Consequently, all potential positions for that wrapped collateral token would be limited by this individual limit, which is significantly smaller compared to what Raft aims to enable.
- Impact: High
- Likelihood: High
If someone donates some wrapped token to the position manager, the share price for that wrapped token collateral would increase in the next update (_updateDebtAndCollateralIndex). Now for all position holders who had opened a position with maxBalance, it's impossible to withdraw or get liquidated. Even if no one donates, this will happen eventually due to redeem rebates. Such an event could cause denial of service for borrowers' positions opened close to maxBalance.
- Impact: Medium
- Likelihood: High
At first, There would be a certain limit you can redeem for one address.
After some time, once the balance becomes > maxBalance for the fee recipient, the protocol won't be able to accept redemption fees anymore, blocking all redemptions. Using this, anyone can DOS redemptions temporarily by sending maxBalance - balanceOf(recipient) of the wrapped token to feeRecipient directly.
- Impact: Low
- Likelihood: High
Liquidation bots are usually run by only a certain set of people with specific addresses. Now, there would be a limit up to which they can liquidate positions. As per the current state of the code, liquidation bots will need to create new addresses with each reach of the limit, transfer ETH there, and then liquidate.
Consider skipping max balance check for all transfers from position manager.
Fixed By raft-fi/contracts#420 raft-fi/contracts#421 raft-fi/contracts#422
Raft Team is enforcing limit solely on deposits with this fix and is using a whitelist for the caller. This caller is intended to be one of Raft's other peripheral contracts, rather than standard user addresses. Further, they have blocked the transfer of wrapped tokens, unless the process is initiated by the position manager.
[Q-01] SafeERC20 extension not used for _underlyingCollateralToken inside PositionManagerWrappedCollateralToken
_underlyingCollateralToken.transferFrom(msg.sender, address(this), collateralChange); _underlyingCollateralToken.approve(address(wrappedCollateralToken), type(uint256).max);
If you are planning to use tokens who return boolean instead of reverting as underlying, consider using SafeERC20 extension.
Fixed by raft-fi/contracts#424
[Q-02] Check Effects Interaction pattern not followed for managePositionETH() of PositionManagerWETH
L65: if (!isCollateralIncrease && collateralChange > 0)
PositionManagerWETH.sol
managePositionETH()
L65
if (!isCollateralIncrease && collateralChange > 0) {
wrappedCollateralToken.withdrawTo(address(this), collateralChange);
IWETH(address(_underlyingCollateralToken)).withdraw(collateralChange);
(bool success,) = msg.sender.call{ value: collateralChange }(""); // @audit action 1: control transferred to the caller
if (!success) {
revert SendingEtherFailed();
}
}
if (isDebtIncrease) {
_rToken.transfer(msg.sender, debtChange); // @audit action 2
}There is no incentive to renter here, since this contract is not designed to hold any funds, however it's still recommended to follow check effect interaction pattern.
Consider doing rToken transfer before ETH transfer.
Fixed by raft-fi/contracts#426
[Q-03] Case of debtChange == type(uint256).max not accounted for in managePositionETH() of PositionManagerStETH
All of Raft's manage position methods on its position manager wrappers have a feature where a user can pass debtChange as the uint256 maximum, after which the total debt of the user is considered for debt decrease.
However, this has not been implemented for managePositionETH of PositionManagerStETH.
PositionManagerStETH.sol
managePositionETH()
if (!isDebtIncrease) {
_applyPermit(_rToken, permitSignature);
_rToken.transferFrom(msg.sender, address(this), debtChange);
}
managePositionStETH()
if (!isDebtIncrease) {
if (debtChange == type(uint256).max) {
debtChange = _raftDebtToken.balanceOf(msg.sender);
}
_applyPermit(_rToken, permitSignature);
_rToken.transferFrom(msg.sender, address(this), debtChange);
}Consider adding this feature for managePositionETH() of PositionManagerStETH as well.
Fixed by raft-fi/contracts#427
Raft added a check within the position manager to account for the potential truncation caused by division:
L249 totalCollateral - collateralToRedeem == 0
if (
totalCollateral - collateralToRedeem == 0
|| totalCollateral - collateralToRedeem < lowTotalDebt.divDown(price)
) {
revert TotalCollateralCannotBeLowerThanMinCollateral(
collateralToken, totalCollateral - collateralToRedeem, lowTotalDebt.divDown(price)
);
}However, the check totalCollateral - collateralToRedeem == 0 seems unnecessary, given that the probability of lowTotalDebt.divDown(price) equating to 0 is virtually null.
As a result, the second condition would invariably encompass the first, making it redundant.
If you consider chances of lowTotalDebt.divDown(price) equating to 0, nonexistent as well, consider removing the
totalCollateral - collateralToRedeem == 0 check.
Won't Do
Raft Team: "In our opinion, we shouldn't delete this, gas optimization is too small for this, and because of collateralInfo[collateralToken].splitLiquidation.LOW_TOTAL_DEBT(); which could be changed (or set to any value), this part lowTotalDebt.divDown(price) could be equal to zero (I completely agree that this will be very unlikely, but, as I wrote, on the other side, gas optimization is too small to remove this). And one more thing, this happens in function which is not called so usually"
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.