HIGH: 3
MED: 2
HIGH: When lender consents before borrower in ETH credit token, all the lent funds are permanently lost.
Description
The addCredit() function transfers money from lender to a LineOfCredit contract, and opens a credit account. increaseCredit() transfers additional funds to an existing credit account contract. Both functions are payable and guarded by mutualConsent(), which guarantees that both borrower and lender approve the execution.
It is not critical to understand how the mutualConsent() modifier works, but it is important to realize both borrower and lender call this function once (the order does not matter), and in the second execution the function body is actually executed.
modifier mutualConsent(address _signerOne, address _signerTwo) {
if(_mutualConsent(_signerOne, _signerTwo)) {
// Run whatever code needed 2/2 consent
_;
}
}The issue is that these two functions have to verify that lender calls after borrower. In the case of ETH credit token, when lender calls first, the message value is sent to the line without bookkeeping. At this point, it doesn't even matter if borrower will call the function as well or not, in both scenarios the previous msg.value will be lost:
function addCredit(
uint128 drate,
uint128 frate,
uint256 amount,
address token,
address lender
)
external
payable
override
whileActive
mutualConsent(lender, borrower)
returns (bytes32)
{
LineLib.receiveTokenOrETH(token, lender, amount);
bytes32 id = _createCredit(lender, token, amount);
require(interestRate.setRate(id, drate, frate));
return id;
}
function receiveTokenOrETH(
address token,
address sender,
uint256 amount
)
external
returns (bool)
{
if(token == address(0)) { revert TransferFailed(); }
if(token != Denominations.ETH) { // ERC20
IERC20(token).safeTransferFrom(sender, address(this), amount);
} else { // ETH
// ---------- ISSUE --------- IF BORROWER CALLS SECOND msg.value will be 0
if(msg.value < amount) { revert TransferFailed(); }
}
return true;
}Note that this is different from the other addCredit/increaseCredit submission (borrower leaks funds) - here the root cause is the order of transactions, whereas in the other submission it's the payability of borrower.
There is no way (to my knowledge) of retrieving funds that are not accounted for in bookkeeping. Even the sweeping function requires the sweeped funds to be in the unusedTokens array:
function sweep(address to, address token) external nonReentrant returns (uint256) {
uint256 amount = unusedTokens[token];
delete unusedTokens[token];
bool success = SpigotedLineLib.sweep(
to,
token,
amount,
_updateStatus(_healthcheck()),
borrower,
arbiter
);
return success ? amount : 0;
}Impact
When lender consents before borrower in ETH credit token, all the lent funds are permanently lost.
Recommended mitigation steps:
Two options:
1. Add a borrowerBeforeLender modifier which will guarantee correct chain of events - the downside is that the order is now forced
2. If lender consents before borrower, keep their funds in escrow and allow them to cancel the consent.
HIGH: Borrower can craft a borrow that cannot be liquidated, even by arbiter.
Description
LineOfCredit manages an array of open credit line identifiers called ids. Many interactions with the Line operate on ids[0], which is presumed to be the oldest borrow which has non zero principal. For example, borrowers must first deposit and repay to ids[0] before other credit lines.
The list is managed by several functions:
CreditListLib.removePosition - deletes parameter id in the ids array
CreditListLib.stepQ - rotates all ids members one to the left, with the leftmost becoming the last element
_sortIntoQ - most complex function, finds the smallest index which can swap identifiers with the parameter id, which satisfies the conditions:
target index is not empty
there is no principal owed for the target index's credit
The idea I had is that if we could corrupt the ids array so that ids[0] would be zero, but after it there would be some other active borrows, it would be a very severe situation. The whileBorrowing() modifier assumes if the first element has no principal, borrower is not borrowing.
modifier whileBorrowing() {
if(count == 0 || credits[ids[0]].principal == 0) { revert NotBorrowing(); }
_;
}It turns out there is a simple sequence of calls which allows borrowing while ids[0] is deleted, and does not re-arrange the new borrow into ids[0]!
id1 = addCredit() - add a new credit line, a new id is pushed to the end of ids array.
id2 = addCredit() - called again, ids.length = 2
close(id1) - calls removePosition() on id1, now ids array is [0x000000000000000000000000, id2 ]
borrow(id2) - will borrow from id2 and call _sortIntoQ. The sorting loop will not find another index other than id2's existing index (id == bytes32(0) is true).
From this sequence, we achieve a borrow while ids[0] is 0! Therefore, credits[ids[0]].principal = credits[0].principal = 0, and whileBorrowing() reverts.
The impact is massive - the following functions are disabled:
SecureLine::liquidate()
LineOfCredit::depositAndClose()
LineOfCredit::depositAndRepay()
LineOfCredit::claimAndRepay()
LineOfCredit::claimAndTrade()
Impact
Borrower can craft a borrow that cannot be liquidated, even by arbiter. Alternatively, functionality may be completely impaired through no fault of users.
Proof of Concept
function _addCreditLender2(address token, uint256 amount) public {
// Prepare lender 2 operations, does same as mintAndApprove()
address lender2 = address(21);
deal(lender2, mintAmount);
supportedToken1.mint(lender2, mintAmount);
supportedToken2.mint(lender2, mintAmount);
unsupportedToken.mint(lender2, mintAmount);
vm.startPrank(lender2);
supportedToken1.approve(address(line), MAX_INT);
supportedToken2.approve(address(line), MAX_INT);
unsupportedToken.approve(address(line), MAX_INT);
vm.stopPrank();
// addCredit logic
vm.prank(borrower);
line.addCredit(dRate, fRate, amount, token, lender2);
vm.stopPrank();
vm.prank(lender2);
line.addCredit(dRate, fRate, amount, token, lender2);
vm.stopPrank();
}
function test_attackUnliquidatable() public {
bytes32 id_1;
bytes32 id_2;
_addCredit(address(supportedToken1), 1 ether);
_addCreditLender2(address(supportedToken1), 1 ether);
id_1 = line.ids(0);
id_2 = line.ids(1);
hoax(borrower);
line.close(id_1);
hoax(borrower);
line.borrow(id_2, 1 ether);
id_1 = line.ids(0);
id_2 = line.ids(1);
console.log("id1 : ", uint256(id_1));
console.log("id2 : ", uint256(id_2));
vm.warp(ttl+1);
assert(line.healthcheck() == LineLib.STATUS.LIQUIDATABLE);
vm.expectRevert(ILineOfCredit.NotBorrowing.selector);
bool isSolvent = line.declareInsolvent();
}Recommended mitigation steps
When sorting new borrows into the ids queue, do not skip any elements.
HIGH: When borrower repays, it can overflow and make them owe 2^256 tokens to lender.
Description
CreditLib's repay() function is the actual accounting of repayments in a LineOfCredit:
function repay(
ILineOfCredit.Credit memory credit,
bytes32 id,
uint256 amount
)
external
returns (ILineOfCredit.Credit memory)
{ unchecked {
if (amount <= credit.interestAccrued) {
credit.interestAccrued -= amount;
credit.interestRepaid += amount;
emit RepayInterest(id, amount);
return credit;
} else {
uint256 interest = credit.interestAccrued;
uint256 principalPayment = amount - interest;
// update individual credit line denominated in token
credit.principal -= principalPayment;
credit.interestRepaid += interest;
credit.interestAccrued = 0;
emit RepayInterest(id, interest);
emit RepayPrincipal(id, principalPayment);
return credit;
}
} }Note that the entire function has no overflow protection. It is assumed that the caller of repay() will perform the necessary validation:
require(amount <= credit.principal + credit.interestAccrued);If this is not the case, the following line will overflow:
credit.principal -= principalPayment;Unfortunately, there is one instance where amount is not checked, in useAndRepay():
/// see ISpigotedLine.useAndRepay
function useAndRepay(uint256 amount) external whileBorrowing returns(bool) {
bytes32 id = ids[0];
Credit memory credit = credits[id];
if (msg.sender != borrower && msg.sender != credit.lender) {
revert CallerAccessDenied();
}
require(amount <= unusedTokens[credit.token]);
unusedTokens[credit.token] -= amount;
credits[id] = _repay(_accrue(credit, id), id, amount);
emit RevenuePayment(credit.token, amount);
return true;
}When borrower tries to repay using unusedTokens, if they request a repay amount smaller than the unusedTokens total, but larger than the maximum they owe to lender, it will overflow the principal field. At this point, borrower will owe close to 2^256 tokens to lender.
Note that it is very easy for user to enter a larger amount than currently owed, as interest is ever accruing and they may estimate it to grow faster than it did in practice until the block is executed.
Impact
When borrower repays, it can overflow and make them owe 2^256 tokens to lender.
Proof of Concept
Copy the following code to SpigotedLine.t.sol:
function test_lender_can_use_and_repay_overflow() public {
deal(address(lender), lentAmount + 1 ether);
deal(address(revenueToken), MAX_REVENUE);
address revenueC = address(0xbeef); // need new spigot for testing
bytes32 id = _createCredit(address(revenueToken), Denominations.ETH, revenueC);
_borrow(id, lentAmount);
bytes memory tradeData = abi.encodeWithSignature(
'trade(address,address,uint256,uint256)',
address(revenueToken),
Denominations.ETH,
2 gwei,
lentAmount + 1 ether
);
uint256 new_tokens;
hoax(borrower);
new_tokens = line.claimAndTrade(address(revenueToken), tradeData);
console.log(new_tokens);
vm.prank(lender); // prank lender
line.useAndRepay(lentAmount + 1 ether);
(, uint256 principal,,,,,) = line.credits(line.ids(0));
console.log("Principal is", principal);
//assertEq(principal, 0);
}Recommended mitigation steps:
It is recommended to check for overflow in repay() function, as there is always a risk of introducing threats when adding more calls to repay().
MED: Borrower can bypass all revenue protection for lender by using custom 0x data
Description
SpigotedLineLib's claimAndTrade function allows borrower to choose when to swap their revenue tokens to credit tokens.
function claimAndTrade(
address claimToken,
address targetToken,
address payable swapTarget,
address spigot,
uint256 unused,
bytes calldata zeroExTradeData
)
external
returns(uint256, uint256)
{
// can't trade into same token. causes double count for unused tokens
if(claimToken == targetToken) { revert BadTradingPair(); }
// snapshot token balances now to diff after trade executes
uint256 oldClaimTokens = LineLib.getBalance(claimToken);
uint256 oldTargetTokens = LineLib.getBalance(targetToken);
// @dev claim has to be called after we get balance
// reverts if there are no tokens to claim
uint256 claimed = ISpigot(spigot).claimEscrow(claimToken);
trade(
claimed + unused,
claimToken,
swapTarget,
zeroExTradeData
);
// underflow revert ensures we have more tokens than we started with
uint256 tokensBought = LineLib.getBalance(targetToken) - oldTargetTokens;
if(tokensBought == 0) { revert TradeFailed(); } // ensure tokens bought
uint256 newClaimTokens = LineLib.getBalance(claimToken);
// ideally we could use oracle to calculate # of tokens to receive
// but sellToken might not have oracle. buyToken must have oracle
emit TradeSpigotRevenue(
claimToken,
claimed,
targetToken,
tokensBought
);
// used reserve revenue to repay debt
if(oldClaimTokens > newClaimTokens) {
uint256 diff = oldClaimTokens - newClaimTokens;
// used more tokens than we had in revenue reserves.
// prevent borrower from pulling idle lender funds to repay other lenders
if(diff > unused) revert UsedExcessTokens(claimToken, unused);
// reduce reserves by consumed amount
else return (
tokensBought,
unused - diff
);
} else { unchecked {
// excess revenue in trade. store in reserves
return (
tokensBought,
unused + (newClaimTokens - oldClaimTokens)
);
} }
}Borrower passes calldata which is sent to 0x DEX via the trade() call:
function trade(
uint256 amount,
address sellToken,
address payable swapTarget,
bytes calldata zeroExTradeData
)
public
returns(bool)
{
if (sellToken == Denominations.ETH) {
// if claiming/trading eth send as msg.value to dex
(bool success, ) = swapTarget.call{value: amount}(zeroExTradeData);
if(!success) { revert TradeFailed(); }
} else {
IERC20(sellToken).approve(swapTarget, amount);
(bool success, ) = swapTarget.call(zeroExTradeData);
if(!success) { revert TradeFailed(); }
}
return true;
}The critical issue is that borrower may use this functionality to exfiltrate their locked escrow funds out of the contract. Borrower has complete freedom on how to craft 0x DEX calldata. They have the entire escrow balance + unused balance to pass to 0x.
The only limitation is that tokensBought (credit tokens) must increase:
// underflow revert ensures we have more tokens than we started with
uint256 tokensBought = LineLib.getBalance(targetToken) - oldTargetTokens;
if(tokensBought == 0) { revert TradeFailed(); } // ensure tokens boughtThere are many different ways to structure the 0x call to exfiltrate the money. The simplest is probably the fillLimitOrder handler in 0x
function fillLimitOrder(
LibNativeOrder.LimitOrder memory order,
LibSignature.Signature memory signature,
uint128 takerTokenFillAmount
) public payable returns (uint128 takerTokenFilledAmount, uint128 makerTokenFilledAmount) {
FillNativeOrderResults memory results = _fillLimitOrderPrivate(
FillLimitOrderPrivateParams({
order: order,
signature: signature,
takerTokenFillAmount: takerTokenFillAmount,
taker: msg.sender,
sender: msg.sender
})
);
LibNativeOrder.refundExcessProtocolFeeToSender(results.ethProtocolFeePaid);
(takerTokenFilledAmount, makerTokenFilledAmount) = (
results.takerTokenFilledAmount,
results.makerTokenFilledAmount
);
}User specifies this struct for processing:
struct LimitOrder {
IERC20TokenV06 makerToken;
IERC20TokenV06 takerToken;
uint128 makerAmount;
uint128 takerAmount;
uint128 takerTokenFeeAmount;
address maker;
address taker;
address sender;
address feeRecipient;
bytes32 pool;
uint64 expiry;
uint256 salt;
}Therefore, it is easy for borrower to prepare a trade as maker, which accepts a ton of revenue tokens, and returns a single credit token. From the trade() call, borrower will fulfill that order as a taker, and still pass the balance check performed later.
There are many more clever and devious ways to exfiltrate the money once borrower has 0x arbitrary call primitive, for example playing with the feeRecipient comes to mind.
Impact
Borrower can bypass all revenue protection for lender, essentially borrowing for free.
Recommended mitigation steps:
If it is desired to allow borrower to arbitrarily call 0x, it must be locked down very hard. Need to run balance checks on all revenue tokens, check slippage is acceptable and so on.
MED: When using ETH as credit token, there will be a guaranteed leak of value.
Description
The receiveTokenOrETH() utility function is used multiple times the LineOfCredit contract:
addCredit
increaseCredit
depositAndClose
depositAndRepay
close
Payment is accepted in ETH from the contract call, or ERC20 transfer:
function receiveTokenOrETH(
address token,
address sender,
uint256 amount
)
external
returns (bool)
{
if(token == address(0)) { revert TransferFailed(); }
if(token != Denominations.ETH) { // ERC20
IERC20(token).safeTransferFrom(sender, address(this), amount);
} else { // ETH
if(msg.value < amount) { revert TransferFailed(); }
}
return true;
}The issue is that whenever the caller sends more msg.value than amount to be received, the difference would not be refunded back to them and be permanently stuck in the contract.
Usually leaks of value are treated as Med severity, but in this instance the leak is guaranteed in the close() and depositAndClose() function. This is because user does not specify an amount, only that they wish to close the credit line. The amount is calculated in real time, using the latest accrued interest. Therefore, user cannot know the accurate amount of msg.value to send and will either send too little, making the transaction revert, or send too much, and leak the difference. Because the leak is unavoidable, I believe the finding to be of high severity.
Impact
When using ETH as credit token, there will be a guaranteed leak of value.
Recommended mitigation steps:
receiveTokenOrETH() should receive a boolean refundSpare which will send spare ETH to the msg.sender. It should only be false if the caller wants to perform several receives and wishes to perform accounting in the calling function.
Comments