cccz - ERC5095.mint should ensure the user received at least the amount desired

[github-actions[bot]]

cccz

medium

ERC5095.mint should ensure the user received at least the amount desired

Summary

ERC5095.mint should ensure the user received at least the amount desired

Vulnerability Detail

In ERC5095.deposit, ensure that the number of PTs received by the user must be greater than the number of UToken by checking returned >= a. This is used to ensure that the user does not suffer a loss in the deposit.

        // Ensure the user received at least the amount desired
        if (returned < a) {
            revert Exception(16, returned, a, address(0), address(0));
        }

However, this is not done in ERC5095.mint, which leads to the possibility that the number of PTs minted by the mint function is less than the number of UToken, thus exposing the user to losses.

    function mint(address r, uint256 s) external override returns (uint256) {
        // Revert if called at or after maturity
        if (block.timestamp >= maturity) {
            revert Exception(
                21,
                block.timestamp,
                maturity,
                address(0),
                address(0)
            );
        }

        // Calculate how much underlying will be needed to mint the desired shares
        uint128 assets = Cast.u128(previewMint(s));

        // Transfer the underlying to the token
        Safe.transferFrom(
            IERC20(underlying),
            msg.sender,
            address(this),
            assets
        );

        // Swap the underlying for principal tokens via the pool
        uint128 returned = IMarketPlace(marketplace).sellUnderlying(
            underlying,
            maturity,
            assets,
            Cast.u128(s - (s / 100))
        );

        // Transfer the principal tokens to the desired receiver
        _transfer(address(this), r, returned);

        return returned;
    }

Impact

It leads to the possibility that the number of PTs minted by the mint function is less than the number of UToken, thus exposing the user to losses.

Code Snippet

https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L199-L203 https://github.com/sherlock-audit/2023-01-illuminate/blob/main/src/tokens/ERC5095.sol#L214-L249

Tool used

Manual Review

Recommendation

Change to

    function mint(address r, uint256 s) external override returns (uint256) {
        // Revert if called at or after maturity
        if (block.timestamp >= maturity) {
            revert Exception(
                21,
                block.timestamp,
                maturity,
                address(0),
                address(0)
            );
        }

        // Calculate how much underlying will be needed to mint the desired shares
        uint128 assets = Cast.u128(previewMint(s));

        // Transfer the underlying to the token
        Safe.transferFrom(
            IERC20(underlying),
            msg.sender,
            address(this),
            assets
        );

        // Swap the underlying for principal tokens via the pool
        uint128 returned = IMarketPlace(marketplace).sellUnderlying(
            underlying,
            maturity,
            assets,
            Cast.u128(s - (s / 100))
        );
+        if (returned < assets) {
+            revert Exception(16, returned, a, address(0), address(0));
+        }
        // Transfer the principal tokens to the desired receiver
        _transfer(address(this), r, returned);

        return returned;
    }

[IllIllI000]

See the discussion in https://github.com/sherlock-audit/2023-01-illuminate-judging/issues/16 about where slippage of this sort should be. Seem invalid

[sourabhmarathe]

Duplicate of #16.

[hrishibhat]

After further discussion with the Lead Watson and the Sponsor, this issue is not considered a duplicate of issue #16 As the contract requires compliance with ERC4626 for further external integrations, the slippage in the above function is supposed to be checked by the caller. The only functions where a slippage check is expected by the ERC are withdraw/redeem as mentioned in issue 16. Hence this issue is not considered a valid medium/high.