External PT redeem functions can be reentered to double count the received underlying funds

Summary

There are two versions of external PT redeem() functions in Redeemer: multi-PT one and Sense. Both calculated underlying funds returned from redeem as a balance difference, both can be reentered, and Sense one calls user-supplied adapter to perform the redeem.

An attacker can create a pre-cooked contract that first calls redeem() for all other types of PT, then proceeds to call the correct Sense adapter. All the funds obtained from PT except Sense will be double counted. The attacker will now need to burn its Illuminate PTs first, obtaining the major part of the underlying funds and making the contract insolvent for all other users.

Vulnerability Detail

Bob the attacker will:

Lend 1 block before maturity, wait 1 block, then run the following with elevated gas cost
Create wrapper 'a', that do call Sense pool, but before that calls multi-PT redeem with all PTs available on Lender's balance the corresponding number of times
Call correct Sense adapter to redeem. Result is that all besides Sense is double counted in holdings
Be first to redeem Illuminate PTs, obtaining up to double amount of the funds
Other users will soon be unable to redeem due to the lack of underlying funds in the contract

Impact

Bob will be able to extract the most part of the total underlying funds, making the contract up to insolvent for all other users.

It can be done without any additional requirements, so setting the severity to be high.

Code Snippet

Swivel, Yield, Element, Pendle, APWine, Tempus and Notional (multi-PT) redeem() isn't protected from reentracy:

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

    /// @notice redeem method for Swivel, Yield, Element, Pendle, APWine, Tempus and Notional protocols
    /// @param p principal value according to the MarketPlace's Principals Enum
    /// @param u address of an underlying asset
    /// @param m maturity (timestamp) of the market
    /// @return bool true if the redemption was successful
    function redeem(
        uint8 p,
        address u,
        uint256 m
    ) external returns (bool) {
        // Get the principal token that is being redeemed by the user
        address principal = IMarketPlace(marketPlace).token(u, m, p);

        // Get the maturity for the given principal token
        uint256 maturity;
        if (p == uint8(MarketPlace.Principals.Swivel)) {
            maturity = ISwivelToken(principal).maturity();
        } else if (p == uint8(MarketPlace.Principals.Yield)) {
            maturity = IYieldToken(principal).maturity();
        } else if (p == uint8(MarketPlace.Principals.Element)) {
            maturity = IElementToken(principal).unlockTimestamp();
        } else if (p == uint8(MarketPlace.Principals.Pendle)) {
            maturity = IPendleToken(principal).expiry();
        } else if (p == uint8(MarketPlace.Principals.Tempus)) {
            maturity = ITempusPool(ITempusToken(principal).pool())
                .maturityTime();
        } else if (p == uint8(MarketPlace.Principals.Apwine)) {
            // APWine's maturity is retrieved indirectly via the PT's
            // futureVault and Controller
            address futureVault = IAPWineToken(principal).futureVault();

            address controller = IAPWineFutureVault(futureVault)
                .getControllerAddress();

            uint256 duration = IAPWineFutureVault(futureVault)
                .PERIOD_DURATION();

            maturity = IAPWineController(controller).getNextPeriodStart(
                duration
            );
        } else if (p == uint8(MarketPlace.Principals.Notional)) {
            maturity = INotional(principal).getMaturity();
        } else {
            revert Exception(6, p, 0, address(0), address(0));
        }

        // Verify that the token has matured
        if (maturity > block.timestamp) {
            revert Exception(7, maturity, 0, address(0), address(0));
        }

        // Cache the lender to save gas on sload
        address cachedLender = lender;

        // Get the amount to be redeemed
        uint256 amount = IERC20(principal).balanceOf(cachedLender);

        // Receive the principal token from the lender contract
        Safe.transferFrom(
            IERC20(principal),
            cachedLender,
            address(this),
            amount
        );

        // Get the starting balance of the underlying held by the redeemer
        uint256 starting = IERC20(u).balanceOf(address(this));

        if (p == uint8(MarketPlace.Principals.Swivel)) {
            // Redeems principal tokens from Swivel
            if (!ISwivel(swivelAddr).redeemZcToken(u, maturity, amount)) {
                revert Exception(15, 0, 0, address(0), address(0));
            }
        } else if (p == uint8(MarketPlace.Principals.Yield)) {
            // Redeems principal tokens from Yield
            IYieldToken(principal).redeem(address(this), amount);
        } else if (p == uint8(MarketPlace.Principals.Element)) {
            // Redeems principal tokens from Element
            IElementToken(principal).withdrawPrincipal(amount, address(this));
        } else if (p == uint8(MarketPlace.Principals.Pendle)) {
            // Get the forge contract for the principal token
            address forge = IPendleToken(principal).forge();

            // Get the forge ID of the principal token
            bytes32 forgeId = IPendleForge(forge).forgeId();

            // Redeem the tokens from the Pendle contract
            IPendle(pendleAddr).redeemAfterExpiry(forgeId, u, maturity);

            // Get the compounding asset for this market
            address compounding = IPendleToken(principal)
                .underlyingYieldToken();

            // Redeem the compounding to token to the underlying
            IConverter(converter).convert(
                compounding,
                u,
                IERC20(compounding).balanceOf(address(this))
            );
        } else if (p == uint8(MarketPlace.Principals.Tempus)) {
            // Retrieve the pool for the principal token
            address pool = ITempusToken(principal).pool();

            // Redeems principal tokens from Tempus
            ITempus(tempusAddr).redeemToBacking(pool, amount, 0, address(this));
        } else if (p == uint8(MarketPlace.Principals.Apwine)) {
            apwineWithdraw(principal, u, amount);
        } else if (p == uint8(MarketPlace.Principals.Notional)) {
            // Redeems principal tokens from Notional
            INotional(principal).redeem(
                INotional(principal).maxRedeem(address(this)),
                address(this),
                address(this)
            );
        }

        // Calculate how much underlying was redeemed
        uint256 redeemed = IERC20(u).balanceOf(address(this)) - starting;

        // Update the holding for this market
        holdings[u][m] = holdings[u][m] + redeemed;

        emit Redeem(p, u, m, redeemed, msg.sender);
        return true;
    }

Sense redeem() records IERC20(u).balanceOf(address(this)) - starting as recovered underlying funds, calling user-supplied adapter a in-between balance snapshots:

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

    /// @notice redeem method signature for Sense
    /// @param p principal value according to the MarketPlace's Principals Enum
    /// @param u address of an underlying asset
    /// @param m maturity (timestamp) of the market
    /// @param s Sense's maturity is needed to extract the pt address
    /// @param a Sense's adapter for this market
    /// @return bool true if the redemption was successful
    function redeem(
        uint8 p,
        address u,
        uint256 m,
        uint256 s,
        address a
    ) external returns (bool) {
        // Check the principal is Sense
        if (p != uint8(MarketPlace.Principals.Sense)) {
            revert Exception(6, p, 0, address(0), address(0));
        }

        // Get Sense's principal token for this market
        IERC20 token = IERC20(IMarketPlace(marketPlace).token(u, m, p));

        // Cache the lender to save on SLOAD operations
        address cachedLender = lender;

        // Get the balance of tokens to be redeemed by the user
        uint256 amount = token.balanceOf(cachedLender);

        // Transfer the user's tokens to the redeem contract
        Safe.transferFrom(token, cachedLender, address(this), amount);

        // Get the starting balance to verify the amount received afterwards
        uint256 starting = IERC20(u).balanceOf(address(this));

        // Get the divider from the adapter
        ISenseDivider divider = ISenseDivider(ISenseAdapter(a).divider());

        // Redeem the tokens from the Sense contract
        ISenseDivider(divider).redeem(a, s, amount);

        // Get the compounding token that is redeemed by Sense
        address compounding = ISenseAdapter(a).target();

        // Redeem the compounding token back to the underlying
        IConverter(converter).convert(
            compounding,
            u,
            IERC20(compounding).balanceOf(address(this))
        );

        // Get the amount received
        uint256 redeemed = IERC20(u).balanceOf(address(this)) - starting;

        // Verify that underlying are received 1:1 - cannot trust the adapter
        if (redeemed < amount) {
            revert Exception(13, 0, 0, address(0), address(0));
        }

        // Update the holdings for this market
        holdings[u][m] = holdings[u][m] + redeemed;

        emit Redeem(p, u, m, redeemed, msg.sender);
        return true;
    }

Bob will supply a which first call multi-PT version of redeem() above with all available types of PTs, maximizing the underlying output, then calls the correct Sense adapter to obtain the Sense part. All underlying funds from all other types of PTs will be counted twice, first in the multi-PT redeem, second in Sense redeem.

Tool used

Manual Review

Recommendation

If there is no desire to refactor Sense support from the current version, that calls user-supplied contract, consider adding reentrancy guard modifiers to both redeem() functions.

Illuminate redeem modifies the holdings in the opposite direction, but for the sake of reducing the surface altogether consider adding reentrancy guard there as well. Additional cost is well justified in the both cases.

dmitriia commented 2 years ago

Escalate for 50 USDC Lending and redeeming reentrancy surfaces are located in different contracts and do not have exactly same mechanics. This is for redeeming reentrancy and duplicates looks to be #64, #67, #217, #232

This escalation is for the same root cause as both of 179

sherlock-admin commented 2 years ago

Escalate for 50 USDC Lending and redeeming reentrancy surfaces are located in different contracts and do not have exactly same mechanics. This is for redeeming reentrancy and duplicates looks to be #64, #67, #217, #232

This escalation is for the same root cause as both of 179

You've created a valid escalation for 50 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

Evert0x commented 2 years ago

Escalation accepted

sherlock-admin commented 2 years ago

Escalation accepted

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

sherlock-audit / 2022-10-illuminate-judging

hyh - External PT redeem functions can be reentered to double count the received underlying funds #236