This report contains findings reported in the Size competition on code4rena by the me, @flacko, as a part of the Sentryx security team.
| Severity | Issues | Unique |
|---|---|---|
| High | 1 | 0 |
| Medium | 2 | 0 |
https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Liquidate.sol#L96-L99 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Liquidate.sol#L100
Liquidator will always receive a much smaller reward when liquidating loans due to not converting the liquidation fee percent from the debt position's future value to collateral token in liquidate().
When calculating the liquidator reward (liquidatorReward), the smaller value from these two is taken:
- The difference between the part of all the borrower's collateral that the specific debt position's future value represents and the actual value the debt's future value quoted in collateral token
- Liquidation reward percent out of the debt position's future value
The first value is in collateral token (WETH/ETH) which has 18 decimals as the sponsor has stated in the contest readme, the second value however has 6 decimals as its the USDC token, again as the sponsor has stated in the contest readme.
The problem at this point is obvious - comparing USDC and WETH will most of the times favour the USDC value as it has 12 decimals less, so even small WETH values like 0.00001e18 ETH will be more than 10_000e6 USDC (0.00000001 in 18 decimals, which itself is an absurdly high and unrealistic liquidation reward).
function executeLiquidate(State storage state, LiquidateParams calldata params)
external
returns (uint256 liquidatorProfitCollateralToken)
{
DebtPosition storage debtPosition = state.getDebtPosition(params.debtPositionId);
LoanStatus loanStatus = state.getLoanStatus(params.debtPositionId);
uint256 collateralRatio = state.collateralRatio(debtPosition.borrower);
// ...
uint256 assignedCollateral = state.getDebtPositionAssignedCollateral(debtPosition);
uint256 debtInCollateralToken = state.debtTokenAmountToCollateralTokenAmount(debtPosition.futureValue);
uint256 protocolProfitCollateralToken = 0;
// profitable liquidation
if (assignedCollateral > debtInCollateralToken) {
uint256 liquidatorReward = Math.min(
assignedCollateral - debtInCollateralToken,
→ Math.mulDivUp(debtPosition.futureValue, state.feeConfig.liquidationRewardPercent, PERCENT)
);
→ liquidatorProfitCollateralToken = debtInCollateralToken + liquidatorReward;
// ...
} else {
// unprofitable liquidation
liquidatorProfitCollateralToken = assignedCollateral;
}
state.data.borrowAToken.transferFrom(msg.sender, address(this), debtPosition.futureValue);
state.data.collateralToken.transferFrom(debtPosition.borrower, msg.sender, liquidatorProfitCollateralToken);
state.data.collateralToken.transferFrom(
→ debtPosition.borrower, state.feeConfig.feeRecipient, protocolProfitCollateralToken
);
debtPosition.liquidityIndexAtRepayment = state.data.borrowAToken.liquidityIndex();
state.repayDebt(params.debtPositionId, debtPosition.futureValue);
}So having picked the USDC value as its the smaller one, later in executeLiquidate() the liquidator will be transferred liquidatorProfitCollateralToken amount of collateral token which consists of the debt position's credit converted to collateral token (WETH) + the liquidator reward (the USDC value) and this way the liquidator will receive 10**12 times less of a reward than they should.
Manual review
When calculating the liquidation reward percent from the debt position's futureValue, convert it first to collateral token amount.
diff --git a/src/libraries/actions/Liquidate.sol b/src/libraries/actions/Liquidate.sol
index 59b8de1..8d6b4dd 100644
--- a/src/libraries/actions/Liquidate.sol
+++ b/src/libraries/actions/Liquidate.sol
@@ -95,7 +95,11 @@ library Liquidate {
if (assignedCollateral > debtInCollateralToken) {
uint256 liquidatorReward = Math.min(
assignedCollateral - debtInCollateralToken,
- Math.mulDivUp(debtPosition.futureValue, state.feeConfig.liquidationRewardPercent, PERCENT)
+ Math.mulDivUp(
+ state.debtTokenAmountToCollateralTokenAmount(debtPosition.futureValue),
+ state.feeConfig.liquidationRewardPercent,
+ PERCENT
+ )
);
liquidatorProfitCollateralToken = debtInCollateralToken + liquidatorReward;
https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/Multicall.sol#L37 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/Multicall.sol#L40-L42 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Deposit.sol#L80-L82
When deposits are executed normally, a cap is enforced on the borrowAToken total supply. borrowATokens are minted only when underlying borrow token (USDC) is deposited to the protocol.
When depositing via a multicall, however, this cap can be surpassed as this check is disabled.
When executing transactions in a bundle via multicall, a flag is set in storage (state.data.isMulticall) to true
function multicall(State storage state, bytes[] calldata data) internal returns (bytes[] memory results) {
state.data.isMulticall = true;
// ... execute bundled transactions via delegatecall
state.data.isMulticall = false;
}Meaning the validateBorrowATokenCap() function will not be called when executeDeposit() is called.
function executeDeposit(State storage state, DepositParams calldata params) public {
// ....
if (params.token == address(state.data.underlyingBorrowToken)) {
state.depositUnderlyingBorrowTokenToVariablePool(from, params.to, amount);
// borrow aToken cap is not validated in multicall,
// since users must be able to deposit more tokens to repay debt
→ if (!state.data.isMulticall) {
state.validateBorrowATokenCap();
}
} else {
state.depositUnderlyingCollateralToken(from, params.to, amount);
}
// ...
}At the end of the multicall() function, there's another check performed - validateBorrowATokenIncreaseLteDebtTokenDecrease() – but for a totally different thing. It ensures the borrowAToken balance of the Size contract has not risen more than the burned amount of debtToken, where in deposits the former always stays 0 as the borrowATokens are minted to the provided to address and not to the Size contract.
Expand POC
Add the test to test/local/actions/Multicall.t.sol and run it via forge test --match-test test_Multicall_multicall_bypasses_cap_in_all_instances
function test_Multicall_multicall_bypasses_cap_in_all_instances() public {
uint256 cap = 100e6;
_updateConfig("borrowATokenCap", cap);
bytes[] memory data = new bytes[](1);
data[0] = abi.encodeCall(size.deposit, DepositParams({token: address(usdc), amount: cap * 2, to: alice}));
vm.prank(usdc.owner());
usdc.mint(alice, cap * 2);
vm.startPrank(alice);
usdc.approve(address(size), cap * 2);
size.multicall(data);
} Manual review
Remove the borrowATokenCap cap from the risk configuration as it's not effective or enforce it at the end of every multicall() function call, along with the validateBorrowATokenIncreaseLteDebtTokenDecrease() check.
Borrower will not be charged fragmentation fee when creating new credit position when compensating a credit partially
https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Compensate.sol#L136 https://github.com/code-423n4/2024-06-size/blob/main/src/libraries/actions/Compensate.sol#L146-L155
The compensate functionality is used to repay a debt position partially, be it via splitting it into instalments or straightforward repaying it partially using the claimable credit from a credit position that the borrower themselves owns. Out of the total 6 possible scenarios in compensate, in one of them a fragmentation fee should be charged but is not and thus forcing the protocol to spend money out of their pocket for claiming the new credit positions.
When compensating a debt position, the borrower has 2 options:
- Split their debt position in two and repay the debt position they wish
- Use the future cashflow from an existing credit position they own to repay a part of their debt position
There are a total of 6 possible branches of execution in executeCompensate():
- Borrower used part of their CreditPosition to repay DebtPosition entirely - 1 new CreditPosition (
exiterCreditRemainig> 0) - Borrower used part of their CreditPosition to repay DebtPosition partially - 1 new CreditPosition (
exiterCreditRemaining> 0) - Borrower used their entire CreditPosition to repay DebtPosition entirely - 0 new CreditPositions (
exiterCreditRemaining= 0) - Borrower used their entire CreditPosition to repay DebtPosition partially - 0 new CreditPositions (
exiterCreditRemaining= 0) - Borrower repay their entire DebtPosition by creating a new Debt+Credit position - 1 new CreditPosition (
exiterCreditRemaining= 0) (but the old one will have 0 claimable tokens, so 0 new credit positions effectively) - Borrower repays part of their DebtPosition by creating a new Debt+Credit position - 1 new CreditPosition (
exiterCreditRemaining= 0)
In the last branch, the borrower will essentially create a new CreditPosition for the lender to claim and will not be charged as the CreditPosition (creditPositionToCompensate) will be created with credit equal to amountToCompensate, and exiterCreditRemaining is equal to creditPositionToCompensate.credit - amountToCompensate which equals 0 in this case.
At the end of executeCompensate(), it's checked that exiterCreditRemaining is > 0 when deciding whether to charge the borrower the fragmentation fee or not and given that exiterCreditRemaining is 0, the borrower will not be charged a fragmentation fee. On the other side, however, the lender will now have to claim two CreditPositions to collect their principal and interest.
And "fragmentation fee" per the protocol official documentation at the time of the contest means:
2.3.2 Fragmentation Fee
When a lender sells his credit to a new lender, for example, or uses it as future cash flow to borrow, a new CreditPosition is created in the process.
Eventually, the new CreditPosition will become liquidity at the due date. However, this new CreditPosition will also have to be claimed (see the claim chapter for more details), which means one additional transaction to be sent on-chain, for each credit fractionalization.
The Size team intends to run keeper bots to streamline the claim process and aggregate liquidity. However, this operation has some fixed costs in terms of gas, which is why a fixed fee is charged to the user causing the credit split.
Manual review
Probably just also check if creditPositionId == RESERVED_ID && creditPositionWithDebtToRepay.credit > 0 (after fetching the fresh creditPositionWithDebtToRepay from storage to reflect the reduced credit) when deciding whether or not to charge the borrowe