IllIllI - APWine PT redemptions can be blocked forever

[sherlock-admin]

IllIllI

high

APWine PT redemptions can be blocked forever

Summary

APWine PT redemptions can be blocked, causing Illuminate IPTs to be unredeemable

Vulnerability Detail

APWine requires both a PT and a FYT to be provided in order to withdraw funds, and the IRedeemer may not be able to acquire the right number of both. The code assumes that FYTs will be available because the PT will have been rolled into the next period, generating new FYTs. However, the code does not account for malicious users sending extra PTs after the roll, to the Lender, which would mean there is no corresponding FYT available, and the redemption of all APWine PTs for that maturity/underlying combination will fail.

Impact

Permanent freezing of funds

Users that provided their APWine PTs to mint Illuminate PTs (e.g. in order to be an LP in the pool) will have those tokens (their principal) locked forever. Because those users get Illuminate PTs, when it comes time to redeem a specific market, ALL Illuminate PT holders of that market will receieve less than they lent, regardless of whether the original token was an underlying, or a non-APWine PT. The attacker can spend a single wei in order to perform the attack, and they can do so cheaply for every market that has APWine set, by buying one wei of PTs on the open market for each market before the roll, and sending the tokens after the roll.

One method to unblock things would be to buy the right FYTs on the open market, and send the right number back to the contract. However, a well-funded attacker could prevent this by buying up all available supply and have standing market orders for any new supply. One of the reasons for the Illuminate project is to concentrate liquidity since liquidity for these instruments is sparse, so cornering the market is well within the realm of possibility, and after the roll most other users not stuck in the contract will have redeemed their futures, so there will be little to no supply left, and the tokens will be stuck forever.

While the Illuminate project does have an emergency withdraw() function that would allow an admin to rescue the funds and manually distribute them, this would not be trustless and defeats the purpose of having a smart contract.

Code Snippet

The Redeemer fetches the total Lender balance of PTs, and asks to redeem the whole amount (rolled amount + attacker ammount):

// File: src/Redeemer.sol : Redeemer.redeem()   #1

263            // Get the amount to be redeemed
264 @>         uint256 amount = IERC20(principal).balanceOf(cachedLender);
265    
266            // Receive the principal token from the lender contract
267            Safe.transferFrom(
268                IERC20(principal),
269                cachedLender,
270                address(this),
271                amount
272            );
...
314            } else if (p == uint8(MarketPlace.Principals.Apwine)) {
315:@>             apwineWithdraw(principal, u, amount);

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L263-L315

Inside apwineWithdraw(), amount from above becomes a, and that number is used in the call to transferFYTs(), which will fail due to the 'attacker amount' portion:

// File: src/Redeemer.sol : Redeemer.apwineWithdraw()   #2

551        function apwineWithdraw(
552            address p,
553            address u,
554 @>         uint256 a
555        ) internal {
556            // Retrieve the vault which executes the redemption in APWine
557            address futureVault = IAPWineToken(p).futureVault();
558    
559            // Retrieve the controller that will execute the withdrawal
560            address controller = IAPWineFutureVault(futureVault)
561                .getControllerAddress();
562    
563            // Retrieve the next period index
564            uint256 index = IAPWineFutureVault(futureVault).getCurrentPeriodIndex();
565    
566            // Get the FYT address for the current period
567            address fyt = IAPWineFutureVault(futureVault).getFYTofPeriod(index);
568    
569            // Trigger claim to FYTs by executing transfer
570            // Safe.transferFrom(IERC20(fyt), address(lender), address(this), a);
571 @>         ILender(lender).transferFYTs(fyt, a);
572    
573            // Redeem the underlying token from APWine to Illuminate
574:           IAPWineController(controller).withdraw(futureVault, a);

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L551-L574

Redemptions of Illuminate PTs for underlyings is based on shares of each Illuminate PT's totalSupply() of the available underlying, not the expect underlying total:
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L422
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L464
https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Redeemer.sol#L517

Tool used

Manual Review

Recommendation

Do an explicit check for the number of FYTs received during the roll, and only transfer that amount. After the transfer, only withdraw() the minimum of a and the current FYT balance (to use up any FYTs manually transferred to the Redeemer if the attacker tries to undo what they did)

[sourabhmarathe]

After APWine rolls over to a new period, the Lender contract will be receive an equivalent amount of FYTs for each APWine PT it holds. As a result, we do not expect this to be an issue for APWine's redemption process.

[IllIllI000]

@sourabhmarathe the Vulnerability Detail section mentions that the issue occurs when the attacker transfers new PTs after the roll, so the Lender won't get FYTs for those new PTs

[JTraversa]

Yup! Moved over to confirmed, and the suggested solution is pretty elegant! 👍

[sourabhmarathe]

https://github.com/Swivel-Finance/illumigrate/pull/260