sherlock-audit / 2023-01-illuminate-judging

1 stars 0 forks source link

cccz - ERC5095.previewRedeem does not return the calculated result #24

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

cccz

medium

ERC5095.previewRedeem does not return the calculated result

Summary

ERC5095.previewRedeem does not return the calculated result

Vulnerability Detail

Post maturity, ERC5095.previewRedeem calculates the number of underlying tokens to return based on s, but here it returns s directly, not the calculated result

    /// @notice Post or at maturity, simulates the effects of redeemption at the current block. Prior to maturity, returns the amount of `assets from a sale of `shares` in PT from a sale of PT on a YieldSpace AMM.
    /// @param s The amount of principal tokens redeemed in the simulation
    /// @return uint256 The amount of underlying returned by `shares` of PT redemption
    function previewRedeem(uint256 s) public view override returns (uint256) {
        if (block.timestamp >= maturity) {
            // After maturity, the amount redeemed is based on the Redeemer contract's holdings of the underlying
            Cast.u128(
                s *
                    Cast.u128(
                        IRedeemer(redeemer).holdings(underlying, maturity)
                    )
            ) / _totalSupply;
            return s;
        }

        // Prior to maturity, return a a preview of a swap on the pool
        return IYield(pool).sellFYTokenPreview(Cast.u128(s));
    }

Impact

This makes the result returned by previewRedeem incorrect

Code Snippet

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L138-L152

Tool used

Manual Review

Recommendation

Change to

    function previewRedeem(uint256 s) public view override returns (uint256) {
        if (block.timestamp >= maturity) {
            // After maturity, the amount redeemed is based on the Redeemer contract's holdings of the underlying
+          return Cast.u128(
-            Cast.u128(
                s *
                    Cast.u128(
                        IRedeemer(redeemer).holdings(underlying, maturity)
                    )
            ) / _totalSupply;
-            return s;
        }

        // Prior to maturity, return a a preview of a swap on the pool
        return IYield(pool).sellFYTokenPreview(Cast.u128(s));
    }
IllIllI000 commented 1 year ago

A great find, but doesn't state how funds are lost

hrishibhat commented 1 year ago

As the result of previewRedeem does not seem to result in any loss of funds pre-maturity considering this issue as a low.

IllIllI000 commented 1 year ago

https://github.com/Swivel-Finance/illuminate/pull/21 PR properly returns the calculated result, as was outlined in the issue recommendation