bin2chen - withdraw() Logical problem

[sherlock-admin]

bin2chen

high

withdraw() Logical problem

Summary

in ERC509#withdraw() will call sellPrincipalToken() to sell PT get underlying
but not transferred msg.sender to address(ERC5095) first

Vulnerability Detail

in ERC509#withdraw() will call sellPrincipalToken():

    function withdraw(
        uint256 a,
        address r,
        address o
    ) external override returns (uint256) {

        if (block.timestamp < maturity) {
            uint128 shares = Cast.u128(previewWithdraw(a));

            if (o == msg.sender) {
                //******@audit this will sell address(ERC5095)' shares,not msg.sender 's shares,need transfer first******/
                uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
                    underlying,
                    maturity,
                    shares,
                    Cast.u128(a - (a / 100))
                );
......
        else {
...
                _allowance[o][msg.sender] = allowance - shares;
                //******@audit this will sell address(ERC5095)' shares,not o's shares,need transfer first******/
                uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
                    underlying,
                    maturity,
                    Cast.u128(shares),
                    Cast.u128(a - (a / 100))
                );
...

sellPrincipalToken() will sell address(ERC5095)' shares,not msg.sender 's shares or o's shares,need transfer shares to address(ERC5095) first

ps: ERC5095#redeem() Have the same problem.

Impact

If address(ERC5095) has a balance, shares will be lost; if not, the call will fail.

Code Snippet

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L219-L225

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/tokens/ERC5095.sol#L262-L266

Tool used

Manual Review

Recommendation

    function withdraw(
        uint256 a,
        address r,
        address o
    ) external override returns (uint256) {
        // Pre maturity
        if (block.timestamp < maturity) {
            uint128 shares = Cast.u128(previewWithdraw(a));
            // If owner is the sender, sell PT without allowance check
            if (o == msg.sender) {
+               _transfer(msg.sender, address(this), shares);
                uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
                    underlying,
                    maturity,
                    shares,
                    Cast.u128(a - (a / 100))
                );
                Safe.transfer(IERC20(underlying), r, returned);
                return returned;
                // Else, sell PT with allowance check
            } else {
                uint256 allowance = _allowance[o][msg.sender];
                if (allowance < shares) {
                    revert Exception(
                        20,
                        allowance,
                        shares,
                        address(0),
                        address(0)
                    );
                }
                _allowance[o][msg.sender] = allowance - shares;
+              _transfer(msg.sender, address(this), shares);
                uint128 returned = IMarketPlace(marketplace).sellPrincipalToken(
                    underlying,
                    maturity,
                    Cast.u128(shares),
                    Cast.u128(a - (a / 100))
                );
                Safe.transfer(IERC20(underlying), r, returned);
                return returned;
            }
        }

Duplicate of #195

[Evert0x]

@sourabhmarathe is this a duplicate of an issue?

[sourabhmarathe]

@Evert0x Duplicate of #195