The Controller has the _executeCouponTrade() function to trade coupons through the Clober DEX with certain constraints. It is structured as a recursive implementation, where the last coupon is peeled off the array, traded and then through the Clober callback the rest of the array is traded using _executeCouponTrade() again.
The function receives couponsToBuy and couponsToSell arrays. When one of them is not empty, it assumes the other one is (an empty Coupon[] array is passed instead of the actual array).
This assumption holds when users interact with the BorrowController or DepositController. However as Controller was written with inheritance in mind, it needs to handle the generic case when coupons need to be both bought and sold. The buy and sell lists come from the Position calculateCouponRequirement() function. This is the critical code component which determines the coupon array lengths.
Consider the possibility that a user wishes to extend the debt duration and lower the debt amount. In this case, for the epochs up to the previous expiry epoch, a user is eligible for coupons (mintCoupons), after which they will need to burn coupons until the new expiry epoch. Both arrays will not be empty.
The impact will be that the described position changes cannot be implemented in controllers (will revert), although they use the Controller parent code correctly.
Recommended mitigation
Pass the original arrays to the recursive implementation instead of zero-sized arrays in _executeCouponTrade().
Description
The Controller has the
_executeCouponTrade()function to trade coupons through the Clober DEX with certain constraints. It is structured as a recursive implementation, where the last coupon is peeled off the array, traded and then through the Clober callback the rest of the array is traded using_executeCouponTrade()again.https://github.com/clober-dex/coupon-finance/blob/216e0d9463b340ffcc810ecc81957ebe309048ec/contracts/libraries/Controller.sol#L66-L81
The function receives couponsToBuy and couponsToSell arrays. When one of them is not empty, it assumes the other one is (an empty
Coupon[]array is passed instead of the actual array). This assumption holds when users interact with theBorrowControllerorDepositController. However as Controller was written with inheritance in mind, it needs to handle the generic case when coupons need to be both bought and sold. The buy and sell lists come from the PositioncalculateCouponRequirement()function. This is the critical code component which determines the coupon array lengths.https://github.com/clober-dex/coupon-finance/blob/216e0d9463b340ffcc810ecc81957ebe309048ec/contracts/libraries/BondPosition.sol#L42-L54
Consider the possibility that a user wishes to extend the debt duration and lower the debt amount. In this case, for the epochs up to the previous expiry epoch, a user is eligible for coupons (mintCoupons), after which they will need to burn coupons until the new expiry epoch. Both arrays will not be empty. The impact will be that the described position changes cannot be implemented in controllers (will revert), although they use the Controller parent code correctly.
Recommended mitigation
Pass the original arrays to the recursive implementation instead of zero-sized arrays in
_executeCouponTrade().