Summary
When a user withdraws liquidity from the lending pool the amount burnt is the amount passed in to be withdrawn but this is wrong as the user will only be able to withdraw deposited funds and the interest accrued will be lost.
Vulnerability Details
When a user supply assets, they get rtokens that are rebased and increase in value over time but a user will not be able to withdraw their profits as the call will always revert.
* @notice Allows a user to withdraw reserve assets by burning RTokens
* @param amount The amount of reserve assets to withdraw
*/
function withdraw(uint256 amount) external nonReentrant whenNotPaused onlyValidAmount(amount) {
if (withdrawalsPaused) revert WithdrawalsArePaused();
ReserveLibrary.updateReserveState(reserve, rateData);
_ensureLiquidity(amount);
@audit>> (uint256 amountWithdrawn, uint256 amountScaled, uint256 amountUnderlying) = ReserveLibrary.withdraw(
reserve,
rateData,
amount,
msg.sender
);
_rebalanceLiquidity();
emit Withdraw(msg.sender, amountWithdrawn);
}
function withdraw(
ReserveData storage reserve,
ReserveRateData storage rateData,
uint256 amount,
address recipient
) internal returns (uint256 amountWithdrawn, uint256 amountScaled, uint256 amountUnderlying) {
if (amount < 1) revert InvalidAmount();
updateReserveInterests(reserve, rateData);
@audit>> (uint256 burnedScaledAmount, uint256 newTotalSupply, uint256 amountUnderlying) = IRToken(reserve.reserveRTokenAddress).burn(
recipient,
recipient,
amount,
reserve.liquidityIndex
);
@audit>> amountWithdrawn = burnedScaledAmount;
updateInterestRatesAndLiquidity(reserve, rateData, 0, amountUnderlying);
emit Withdraw(recipient, amountUnderlying, burnedScaledAmount);
return (amountUnderlying, burnedScaledAmount, amountUnderlying);
}
function burn(
address from,
address receiverOfUnderlying,
uint256 amount,
uint256 index
) external override onlyReservePool returns (uint256, uint256, uint256) {
if (amount == 0) {
return (0, totalSupply(), 0);
}
@audit>> uint256 userBalance = balanceOf(from);
_userState[from].index = index.toUint128();
if(amount > userBalance){
amount = userBalance;
}
uint256 amountScaled = amount.rayMul(index);
_userState[from].index = index.toUint128();
@audit>> burn >> _burn(from, amount.toUint128());
if (receiverOfUnderlying != address(this)) {
IERC20(_assetAddress).safeTransfer(receiverOfUnderlying, amount);
}
emit Burn(from, receiverOfUnderlying, amount, index);
return (amount, totalSupply(), amount);
}
Alice deposit 1000 USD
index at ray 1e27
ray increase to 1.1e27
Alice decide to 1100 USD since the balance of return 1100 USD has Alice balance
function balanceOf(address account) public view override(ERC20, IERC20) returns (uint256) {
uint256 scaledBalance = super.balanceOf(account);
return scaledBalance.rayMul(ILendingPool(_reservePool).getNormalizedIncome());
}
But the call will revert ,
because in MINT we only minted 1000 rtokens and now we are trying to burn 1100 rtokens.
the balance increase is not considered in the calculation.
The Contract should use the SCALED AMOUNT for minting and burning but instead, we use the amount yet we do not consider a balance increase.
@audit>> revert >> _burn(from, amount.toUint128());
If Alice withdraws 1000 USD
Alice's total balance will be burned and Alice will have 0 amount left
instead of 100 USD from the 1100 USD balance.
LOOK at Aave
Look at the current implementation has done by Aave v3 =>
reserve.updateInterestRates(reserveCache, params.asset, 0, amountToWithdraw);
bool isCollateral = userConfig.isUsingAsCollateral(reserve.id);
if (isCollateral && amountToWithdraw == userBalance) {
userConfig.setUsingAsCollateral(reserve.id, false);
emit ReserveUsedAsCollateralDisabled(params.asset, msg.sender);
}
IAToken(reserveCache.aTokenAddress).burn(
msg.sender,
params.to,
amountToWithdraw,
reserveCache.nextLiquidityIndex
);
burn tokens =>
function burn(
address from,
address receiverOfUnderlying,
uint256 amount,
uint256 index
) external virtual override onlyPool {
@audit >> _burnScaled(from, receiverOfUnderlying, amount, index);
if (receiverOfUnderlying != address(this)) {
IERC20(_underlyingAsset).safeTransfer(receiverOfUnderlying, amount);
}
}
Burning scaled amount as done by Aave ensures that the user can withdraw the profit made from the liquidity index increase.
function _burnScaled(address user, address target, uint256 amount, uint256 index) internal {
@audit >> uint256 amountScaled = amount.rayDiv(index);
require(amountScaled != 0, Errors.INVALID_BURN_AMOUNT);
uint256 scaledBalance = super.balanceOf(user);
uint256 balanceIncrease = scaledBalance.rayMul(index) -
scaledBalance.rayMul(_userState[user].additionalData);
_userState[user].additionalData = index.toUint128();
@audit >> burn scaled >> _burn(user, amountScaled.toUint128());
if (balanceIncrease > amount) {
uint256 amountToMint = balanceIncrease - amount;
emit Transfer(address(0), user, amountToMint);
emit Mint(user, user, amountToMint, balanceIncrease, index);
} else {
uint256 amountToBurn = amount - balanceIncrease;
emit Transfer(user, address(0), amountToBurn);
emit Burn(user, target, amountToBurn, balanceIncrease, index);
}
}
Impact
User will not be able to withdraw their balance increase as liquidity index increases.
Tools Used
Manual Review
Recommendations
Burn the scaled amount as it has been implemented by Aave, this ensures that we can withdraw both deposit assets and increased balance.
