Arbitrary zeroExTradeData calldata to the exchange router may result in loss of fund for the borrower

[code423n4]

Lines of code

https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L135

Vulnerability details

Impact

During trading claimToken with targetToken, the calldata provided for the 0x router is an arbitrary data that can be provided by the borrower or lender.
https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L135

If lender provides the calldata so that that more claimToken is consumed than expected, the borrower is impacted (loses some fund).

Proof of Concept

If a lender (malicious) would like to claim and repay, the flow is as follows:
claimAndRepay(address claimToken, bytes calldata zeroExTradeData) => function _claimAndTrade(address claimToken, address targetToken, bytes calldata zeroExTradeData) => function claimAndTrade(address claimToken, address targetToken, address payable swapTarget, address spigot, uint256 unused, bytes calldata zeroExTradeData) => function trade(uint256 amount, address sellToken, address payable swapTarget, bytes calldata zeroExTradeData)

https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/modules/credit/SpigotedLine.sol#L93
https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/modules/credit/SpigotedLine.sol#L187
https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L53
https://github.com/debtdao/Line-of-Credit/blob/f32cb3eeb08663f2456bf6e2fba21e964da3e8ae/contracts/utils/SpigotedLineLib.sol#L120

Assuming the targetToken is DAI, and claimToken is USDC, if the calldata zeroExTradeData is something that calls the function buy 100 DAI for any amount of USDC instead of buy 100 DAI for maximum amount of 105 USDC, it is highly possible that more than 105 USDC will be consumed to trade for 100 DAI. In this case, the borrower loses more claimToken than expected.

In summary, since the calldata is arbitrary, the lender can put these data buy X targetToken for any amount of claim token or sell X claimToken for any amount of targetToken, in both cases the borrower may lose fund. While the correct call data is: buy X targetToken for max Y claimToken or sell X claimToken for min Y targetToken.

Tools Used

Recommended Mitigation Steps

The calldata zeroExTradeData should be set in advance that which function of the router is going to be called. For example in case of uniswap router, it should be abi.encodeWithSignature("swapExactTokensForTokens(uint256,uint256,address[],address,uint256)", amountIn, amountOutMin, path, to, dedline). In which the amountIn and amountOutMin should be a factor of each other so the borrower will not lose revenue token a lot.

[c4-judge]

dmvt marked the issue as duplicate of #88

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as partial-50

[C4-Staff]

captainmangoC4 marked the issue as duplicate of #110