Open c4-bot-10 opened 3 months ago
The usage of mulDivDown
instead of rayDiv
was chosen to mitigate Issue 6.3 from this security review https://github.com/spearbit/portfolio/blob/master/pdfs/Size-Spearbit-Security-Review.pdf
Additionally, note how mulDivDown
is safer for the protocol since it discredits the user by at most 1 wei, which is different from what the Aave deposit would give them. In contrast, using rayDiv
could potentially give them 1 wei more in, for example, claim
.
QA is appropriate.
hansfriese changed the severity to QA (Quality Assurance)
hansfriese marked the issue as grade-a
Hi @hansfriese , thanks for your time. It would be greatly appreciated if you could reconsider this issue.
This issue isn't related to the presence of a dust amount
in the protocol
due to a precision error.
The issue with dust amounts is obviously a QA
, but this issue describes a DoS
and potential loss
of users' funds.
In the report, I've also suggested two solutions:
The first is to sync the calculation between Size
and Aave
protocol. (sponsor rejected this)
The second is a straightforward solution to resolve this issue.
Let me re-explain the impact and the second suggestion:
The impact
is that borrowers
will not be able to repay
their debt
in some cases when the borrow cap
is reached.
(Was described in the PoC
).
This can cause a DoS
, and more severely, it can make positions
liquidatable
, leading to user losses
through liquidation
.
Users won't be able to fully repay
their debt
, and sometimes it's impossible to make partial repayments (e.g., when the maturity is in the minimum tenor
: https://github.com/code-423n4/2024-06-size-findings/issues/102).
As a result, positions
become liquidatable
, and users lose
their funds.
I believe the impact
is high.
The second suggestion is to adjust the validation check
to allow repayment
even if there is a 1 wei precision error
.
In my report,
Alternatively, a simple solution could be to incorporate a tolerance of 1 wei in the validation check.
This issue described totally different impacts than other issues like #84.
This PoC is wrong.
The test mistakenly adds more borrow tokens than what is necessary to repay the loan.
Here's a fixed version that does not revert on deposit
+ repay
function test_Multicall_repay_when_borrowAToken_cap() public {
IERC20Metadata debtToken = IERC20Metadata(address(size.data().debtToken));
_setLiquidityIndex(2e27);
uint256 cap = 1000e6;
_updateConfig("borrowATokenCap", cap);
uint256 tenor = 365 days;
_deposit(alice, usdc, cap);
_buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(tenor, 0.03e18));
_deposit(bob, weth, 100e18);
uint256 amount = 100e6 + 1;
uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);
// 103517589 (bob's debt)
uint256 debtAmount = debtToken.balanceOf(bob);
uint256 currentDeposit = size.getUserView(bob).borrowATokenBalance;
uint256 depositRequiredToRepay = debtAmount - currentDeposit;
_mint(address(usdc), bob, depositRequiredToRepay);
_approve(bob, address(usdc), address(size), depositRequiredToRepay);
// aviggiano: the borrower already has some USDC deposited, so no need to deposit more to repay
assertLt(depositRequiredToRepay, debtAmount);
// Bob should always be able to repay his full debt
bytes[] memory data = new bytes[](2);
data[0] = abi.encodeCall(size.deposit, DepositParams({token: address(usdc), amount: depositRequiredToRepay, to: bob}));
data[1] = abi.encodeCall(size.repay, RepayParams({debtPositionId: debtPositionId}));
// aviggiano: multicall does not revert
vm.prank(bob);
size.multicall(data);
// aviggiano: debt is now zero
assertEq(debtToken.balanceOf(bob), 0);
}
Hi @aviggiano , thanks for your review.
The reason is that Bob
didn't withdraw
his borrowed
tokens.
Could you please add the following line to your test?
_withdraw(bob, usdc, size.getUserView(bob).borrowATokenBalance);
Normally, after borrowing
USDC
, borrowers
will withdraw
these tokens to use.
Changed version is as follow:
function test_Multicall_repay_when_borrowAToken_cap() public {
IERC20Metadata debtToken = IERC20Metadata(address(size.data().debtToken));
_setLiquidityIndex(2e27);
uint256 cap = 1000e6;
_updateConfig("borrowATokenCap", cap);
uint256 tenor = 365 days;
_deposit(alice, usdc, cap);
_buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(tenor, 0.03e18));
_deposit(bob, weth, 100e18);
uint256 amount = 100e6 + 1;
uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);
// ether_sky: withdraw
_withdraw(bob, usdc, size.getUserView(bob).borrowATokenBalance);
// 103517589 (bob's debt)
uint256 debtAmount = debtToken.balanceOf(bob);
uint256 currentDeposit = size.getUserView(bob).borrowATokenBalance;
uint256 depositRequiredToRepay = debtAmount - currentDeposit;
_mint(address(usdc), bob, depositRequiredToRepay);
_approve(bob, address(usdc), address(size), depositRequiredToRepay);
// aviggiano: the borrower already has some USDC deposited, so no need to deposit more to repay
// ether_sky: disable this line due to depositRequiredToRepay = debtAmount
// assertLt(depositRequiredToRepay, debtAmount);
// Bob should always be able to repay his full debt
bytes[] memory data = new bytes[](2);
data[0] = abi.encodeCall(size.deposit, DepositParams({token: address(usdc), amount: depositRequiredToRepay, to: bob}));
data[1] = abi.encodeCall(size.repay, RepayParams({debtPositionId: debtPositionId}));
// aviggiano: multicall does not revert
vm.prank(bob);
size.multicall(data);
// aviggiano: debt is now zero
assertEq(debtToken.balanceOf(bob), 0);
}
I got this error as expected.
[FAIL. Reason: BORROW_ATOKEN_INCREASE_EXCEEDS_DEBT_TOKEN_DECREASE(103517590 [1.035e8], 103517589 [1.035e8])]
Note: This builds on my previous issue Users can bypass the borrowAToken cap check by using multicall
. (this is accepted)
To conduct this test, we should implement the suggestion from that report, which involves using the total supply instead of the balance of Size in the multicall cap check.
Hey
Here's a revised PoC that does not revert on multicall
:
function test_Multicall_repay_when_borrowAToken_cap() public {
IERC20Metadata debtToken = IERC20Metadata(address(size.data().debtToken));
_setLiquidityIndex(2e27);
uint256 cap = 1000e6;
_updateConfig("borrowATokenCap", cap);
uint256 tenor = 365 days;
_deposit(alice, usdc, cap);
_buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(tenor, 0.03e18));
_deposit(bob, weth, 100e18);
uint256 amount = 100e6 + 1;
uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);
// ether_sky: withdraw
_withdraw(bob, usdc, size.getUserView(bob).borrowATokenBalance);
// 103517589 (bob's debt)
uint256 debtAmount = debtToken.balanceOf(bob);
uint256 currentDeposit = size.getUserView(bob).borrowATokenBalance;
uint256 depositRequiredToRepay = debtAmount - currentDeposit;
_mint(address(usdc), bob, depositRequiredToRepay);
_approve(bob, address(usdc), address(size), depositRequiredToRepay);
// aviggiano: the borrower already has some USDC deposited, so no need to deposit more to repay
// ether_sky: disable this line due to depositRequiredToRepay = debtAmount
// assertLt(depositRequiredToRepay, debtAmount);
// Bob should always be able to repay his full debt
bytes[] memory data = new bytes[](2);
data[0] = abi.encodeCall(size.deposit, DepositParams({token: address(usdc), amount: depositRequiredToRepay-1, to: bob}));
data[1] = abi.encodeCall(size.repay, RepayParams({debtPositionId: debtPositionId}));
// aviggiano: multicall does not revert
vm.prank(bob);
size.multicall(data);
// aviggiano: debt is now zero
assertEq(debtToken.balanceOf(bob), 0);
}
The reason why your test is reverting is because the liquidity index is not 1 RAY, so during deposit
, the USDC is automatically worth more in terms of aUSDC, and the CapsLibrary
check validates aUSDC amounts, not USDC amounts.
In this case, the user must account for that and deposit a bit less in order not to trigger the BORROW_ATOKEN_INCREASE_EXCEEDS_DEBT_TOKEN_DECREASE
error.
In this specific scenario, simply depositing depositRequiredToRepay-1
does the trick
Thanks again for your time.
The reason why your test is reverting is because the liquidity index is not 1 RAY
Obviously, the liquidity index
in the Aave pool
keeps increasing.
data[0] = abi.encodeCall(size.deposit, DepositParams({token: address(usdc), amount: depositRequiredToRepay-1, to: bob}));
The depositRequiredToRepay
is what the user can get from the view functions to repay
their debt.
Users can attempt to deposit 1 wei
less again if the transaction reverts.
This is a fix from the user's side.
Why doesn't the protocol
provide a perfect solution?
How should users determine whether to deposit the exact amount they get or 1 wei less?
In some cases, users should deposit
the amount they get from the view
functions, and in other cases deposit
1 wei less
to avoid the transaction from reverting.
I believe the normal flow from the user's side is as follows:
repay
their debt
from the view functions.
In our PoC
, this value is depositRequiredToRepay
.revert
reason and reattempt with depositRequiredToRepay - 1
?.
Some users may not know the reason for the transaction reverting.My second suggestion will resolve this issue with a straightforward solution.
In this specific scenario, simply depositing depositRequiredToRepay-1 does the trick
Agree on this, but this is a fix.
The depositRequiredToRepay is what the user can get from the view functions to repay their debt.
Actually, being more specific, depositRequiredToRepay
is not the actual amount users should deposit to repay their debt.
The reason is that DespositTokenLibrary.depositUnderlyingBorrowTokenToVariablePool
expects amount
to be the USDC value (underlying borrow token) that is then converted to szaUSDC (borrowAToken) by depositing to Aave v3. This is explained on the NatSpec:
/// @notice Deposit underlying borrow token to the Size protocol
/// @dev The underlying borrow token is deposited to the Variable Pool,
/// and the corresponding Size borrow token is minted in scaled amounts.
/// @param state The state struct
/// @param from The address from which the underlying borrow token is transferred
/// @param to The address to which the Size borrow token is minted
/// @param amount The amount of underlying borrow token to deposit
function depositUnderlyingBorrowTokenToVariablePool(State storage state, address from, address to, uint256 amount)
external
{
Then, the debt amount is calculated in borrowATokens
, as you can see in AccountingLibrary.createDebtAndCreditPositions
.
This means users' USDC deposits might be slightly different than the reported szaUSDC, because of the scaled/unscaled conversion. Although one could argue that this is unintuitive, it is a very well-documented expected behavior of the protocol that users should be aware of.
In my remediation, I explained how depositing 1 wei less would make the revert go away, just to show that the amount
parameter (in USDC) will different than the deposit token minted value (in szaUSDC).
But another better alternative is possible: simply depositing the total debt and withdrawing the remainder dust amount:
function test_Multicall_repay_when_borrowAToken_cap() public {
IERC20Metadata debtToken = IERC20Metadata(address(size.data().debtToken));
_setLiquidityIndex(2e27);
uint256 cap = 1000e6;
_updateConfig("borrowATokenCap", cap);
uint256 tenor = 365 days;
_deposit(alice, usdc, cap);
_buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(tenor, 0.03e18));
_deposit(bob, weth, 100e18);
uint256 amount = 100e6 + 1;
uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);
_withdraw(bob, usdc, size.getUserView(bob).borrowATokenBalance);
// 103517589 (bob's debt)
uint256 debtAmount = debtToken.balanceOf(bob);
uint256 currentDeposit = size.getUserView(bob).borrowATokenBalance;
uint256 depositRequiredToRepay = debtAmount - currentDeposit;
_mint(address(usdc), bob, depositRequiredToRepay);
_approve(bob, address(usdc), address(size), depositRequiredToRepay);
// Bob should always be able to repay his full debt
bytes[] memory data = new bytes[](3);
data[0] = abi.encodeCall(size.deposit, DepositParams({token: address(usdc), amount: depositRequiredToRepay, to: bob}));
data[1] = abi.encodeCall(size.repay, RepayParams({debtPositionId: debtPositionId}));
data[2] = abi.encodeCall(size.withdraw, WithdrawParams({token: address(usdc), amount: type(uint256).max, to: bob}));
vm.prank(bob);
size.multicall(data);
assertEq(debtToken.balanceOf(bob), 0);
}
As you can see, calling withdraw
at the end of the multicall
with type(uint256).max
will also make sure that the BORROW_ATOKEN_INCREASE_EXCEEDS_DEBT_TOKEN_DECREASE
is not triggered.
You can also create a fuzz test with different liquidity indices and borrow amounts to confirm that this always works for all possible scenarios:
function test_Multicall_repay_when_borrowAToken_cap(uint256 index, uint256 amount) public {
IERC20Metadata debtToken = IERC20Metadata(address(size.data().debtToken));
index = bound(index, 1e27, 2e27);
amount = bound(amount, 100e6, 200e6);
_setLiquidityIndex(index);
uint256 cap = 1000e6;
_updateConfig("borrowATokenCap", cap);
uint256 tenor = 365 days;
_deposit(alice, usdc, cap);
_buyCreditLimit(alice, block.timestamp + 365 days, YieldCurveHelper.pointCurve(tenor, 0.03e18));
_deposit(bob, weth, 100e18);
uint256 debtPositionId = _sellCreditMarket(bob, alice, RESERVED_ID, amount, tenor, false);
_withdraw(bob, usdc, size.getUserView(bob).borrowATokenBalance);
uint256 debtAmount = debtToken.balanceOf(bob);
uint256 currentDeposit = size.getUserView(bob).borrowATokenBalance;
uint256 depositRequiredToRepay = debtAmount - currentDeposit;
_mint(address(usdc), bob, depositRequiredToRepay);
_approve(bob, address(usdc), address(size), depositRequiredToRepay);
bytes[] memory data = new bytes[](3);
data[0] = abi.encodeCall(size.deposit, DepositParams({token: address(usdc), amount: depositRequiredToRepay, to: bob}));
data[1] = abi.encodeCall(size.repay, RepayParams({debtPositionId: debtPositionId}));
data[2] = abi.encodeCall(size.withdraw, WithdrawParams({token: address(usdc), amount: type(uint256).max, to: bob}));
vm.prank(bob);
size.multicall(data);
assertEq(debtToken.balanceOf(bob), 0);
}
Why doesn't the protocol provide a perfect solution?
The reason is that a perfect solution does not exist, because the protocol is tightly coupled, by design, with a third-party integration, Aave v3.
As you know, due to Aave's internal accounting system, multiplications and divisions are rounded to the nearest RAY, which makes it inconsistent. In some cases, users might receive 1 wei more, and in other cases, users may receive 1 wei less.
Both of your recommendations are not perfect either since they also introduce new vulnerabilities:
The current implementation of always rounding down with mulDivDown
is safer than using rayDiv
and rayMul
, as I mentioned, since it will always discredit the user by at most 1 wei, instead of potentially crediting the user by an additional 1 wei. This avoids Denial of Services when pulling assets out of the protocol, such as in claim
or withdraw
.
Thanks for your detailed explanation.
I agree that finding the perfect solution is really difficult for this issue.
But another better alternative is possible: simply depositing the total debt and withdrawing the remainder dust amount:
I believe this is an excellent alternative, but there is an another side case. Users can have several debt positions to repay so they don't want to withdraw the borrowA tokens in the last.
All we are discussing are belongs to the fix.
Anyway, this is my last comment and will let the judge to decide.
Thanks again for your time and great explanation.
Thank you for your detailed discussion. After reconsideration, I still believe QA is more appropriate given the unlikely scenario and the existing resolution for borrowers.
Lines of code
https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/token/NonTransferrableScaledToken.sol#L98-L100 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/DepositTokenLibrary.sol#L60 https://github.com/aave/aave-v3-core/blob/b74526a7bc67a3a117a1963fc871b3eb8cea8435/contracts/protocol/libraries/logic/SupplyLogic.sol#L69-L74 https://github.com/aave/aave-v3-core/blob/b74526a7bc67a3a117a1963fc871b3eb8cea8435/contracts/protocol/tokenization/base/ScaledBalanceTokenBase.sol#L72 https://github.com/aave/aave-v3-core/blob/b74526a7bc67a3a117a1963fc871b3eb8cea8435/contracts/protocol/libraries/math/WadRayMath.sol#L83-L92 https://github.com/code-423n4/2024-06-size/blob/8850e25fb088898e9cf86f9be1c401ad155bea86/src/libraries/CapsLibrary.sol#L35-L40
Vulnerability details
Impact
Users should always be able to
repay
theirdebts
, even if theborrowAToken
cap
has been reached. This is crucial becausepositions
can beliquidated
ifdebts
aren'trepaid
on time, leading to significantlosses
for users. However, in some cases, users cannotrepay
theirdebts
due toprecision errors
. This issue stems fromSize
andAave
using differentmathematical formulas
in the calculation ofscaled balances
.Proof of Concept
Imagine the
borrowAToken
cap
has been reached, and the currentscaled total supply
is denoted asT
. To calculate thetotal supply
, we use the formulaT * L / R
, whereL
represents the currentliquidity index
of theAave pool
andR
is set asRAY
(1e27
).Consider a user who has a
debt D
and wishes torepay
it before maturity. This process should be achievable usingmulticall
transactions, which typically involve two steps:First,
deposit D USDC
intoSize
. TheseD USDC
are supplied into theAave Pool
.In the
Aave pool
, thescaled balance
of theseD USDC
is minted back intoSize
.Aave
employs a slightly differentmathematical approach
to calculatescaled balances
.The
scaled balance
ofD USDC
is given by(D * R + L / 2) / L
.Therefore, the increase in
borrowAToken supply
will be determined by this value.Afterward, the user attempts to
repay
hisdebt
. Obviously thedebt decrease
isD
.In certain cases,
(D * R + L / 2) / L
can exceedD
by just1 wei
. For instance, ifD
is103,517,589
(can occur whenD
is anodd
number) andL
is2e27
(This is possible and can easily occur with different values ofL
), then(D * R + L / 2) / L
becomes103,517,590
. (Please check these values in the below test)As a result of this
precision discrepancy
, the validation check will fail, preventing the user fromrepaying
theirdebt
as intended.Please add below test to the
test/local/actions/Multicall.t.sol
:Note: This builds on my previous issue
Users can bypass the borrowAToken cap check by using multicall
. To conduct this test, we should implement the suggestion from thatreport
, which involves using thetotal supply
instead of thebalance of Size
in themulticall cap check
.Tools Used
Recommended Mitigation Steps
To ensure consistency, aligning
Size
withAave
's calculation methods would be beneficial, though potential changes require careful consideration. Alternatively, a simple solution could be to incorporate a tolerance of1 wei
in thevalidation check
.Assessed type
Math