Griefing attack can block Sense withdrawal altogether

Summary

As Sense redeem() version operates Lender's balance, i.e. calls Sense's divider with the amount equal to lender's balance of Sense PTs, while Sense's adapter is user supplied, a griefing attack is possible by calling redeem() with fake precooked adapter that answer all the performed requests without reverting, but doesn't do anything else (i.e. no real operations, just noops without reverting).

The result of that is no Sense PT redeem is then possible with all current system supply of IMarketPlace(marketPlace).token(u, m, p) to be frozen on Redeemer's balance.

Vulnerability Detail

Sense version of Redeemer's redeem() can be called with fake Sense's adapter a, that comply to the interface, provides divider, but do nothing else, so redeem() completes successfully. The result will be freeze of the total Sense PT system holdings for that maturity and underlying.

The reason is that redeem() requests specifically lender's balance to be redeemed by divider, but after the attack such balance will be empty, all the PTs be moved to Redeemer, and there is no logic to access them there.

This way subsequent redeem() calls with a correct Sense adapter will yield nothing, and Sense PTs previously retrieved are stuck with Redeemer as there are no other ways to redeem them apart Sense version redeem() that operates lender's balance only.

Impact

User funds will be permanently frozen as underlying cannot be retrieved from Sense's PTs held on Redeemer's balance. There are no prerequisites for the attack, all is needed is system using Sense PTs at any substantial scale.

This is a violation of system core logic leading to permanent and massive fund freeze with no external assumptions, that is cheap to perform as no funds apart from cumulative gas cost are needed from an attacker, so setting the severity to be high.

Code Snippet

Sense redeem() calls SenseDivider with amount equal to lender's balance, which is transferred to Redeemer:

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

    /// @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) {
        ...

        // 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);

        ...

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

If divider doesn't act on that redeem(a, s, amount) call, the subsequent Redeemer's redeem() calls will deal only with current Lender's balance, i.e. the amount of Sense PTs that was initially moved end up being frozen.

This is straightforwardly achievable as divider is essentially user-supplied, coming from divider = ISenseDivider(ISenseAdapter(a).divider()) call, where a is an argument of redeem().

Tool used

Manual Review

Recommendation

Consider utilizing the whole balance of Redeemer as any PTs there are subject to further redeem anyway:

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

        // 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));

+   uint256 senseBalance = token.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);
+       ISenseDivider(divider).redeem(a, s, senseBalance);

IllIllI000 commented 1 year ago

@sourabhmarathe if the user provides a fake adapter/divider, the following statement will cause the txn to revert if the ending amount is zero as the Vulnerability Detail section above describes:

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

sourabhmarathe commented 1 year ago

You are right, I had not considered this possibility. I will update the labels.