Slippage check should happens after the trade in MarketPlace.sol

Summary

Slippage check should happen after the trade in MarketPlace.sol

Vulnerability Detail

In the current implementation,

function sellPricinpleToken, buyPrincipleToken,

function sellUnderlying and function buyUnderlying

check the slippage using previewed amount before the trade.

For function sellPricinpleToken

// Preview amount of underlying received by selling `a` PTs
uint256 expected = pool.sellFYTokenPreview(a);

if (expected < s) {
    revert Exception(16, expected, s, address(0), address(0));
}

For function buyPrincipalToken

// Get the amount of base hypothetically required to purchase `a` PTs
uint128 expected = pool.buyFYTokenPreview(a);

// Verify that the amount needed does not exceed the slippage parameter
if (expected > s) {
    revert Exception(16, expected, 0, address(0), address(0));
}

For function sellUnderlying

// Get the number of PTs received for selling `a` underlying tokens
uint128 expected = pool.sellBasePreview(a);

// Verify slippage does not exceed the one set by the user
if (expected < s) {
    revert Exception(16, expected, 0, address(0), address(0));
}

For function buyUnderlying

// Get the amount of PTs hypothetically required to purchase `a` underlying
uint256 expected = pool.buyBasePreview(a);

// Verify that the amount needed does not exceed the slippage parameter
if (expected > s) {
    revert Exception(16, expected, 0, address(0), address(0));
}

However, we cannot use the previewed data as the source of the truth for slippage check,

we should use the actual received amount after the trade to check the slippage in case there is a discrepancy between the previewed amount and the received amount.

Impact

If there is a discrepancy between the previewed amount and the received amount, the slippage limit failed to protect user.

Code Snippet

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Marketplace.sol#L293-L300

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Marketplace.sol#L332-L339

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Marketplace.sol#L369-L377

https://github.com/sherlock-audit/2022-10-illuminate/blob/main/src/Marketplace.sol#L403-L411

Tool used

Manual Review

Recommendation

We recommend the project use balance before and balance after to check how much fund the user received,

then apply a slippage check. In fact, the code in Lender.sol and Redeem.sol are already sticked to this implementation to protect users.

Great job! I believe the same efforts are needed in the Marketplace.sol!

Using sellUnderlying as an example:

The project can change from the implementation

    function sellUnderlying(
        address u,
        uint256 m,
        uint128 a,
        uint128 s
    ) external returns (uint128) {
        // Get the pool for the market
        IPool pool = IPool(pools[u][m]);

        // Get the number of PTs received for selling `a` underlying tokens
        uint128 expected = pool.sellBasePreview(a);

        // Verify slippage does not exceed the one set by the user
        if (expected < s) {
            revert Exception(16, expected, 0, address(0), address(0));
        }

        // Transfer the underlying tokens to the pool
        Safe.transferFrom(IERC20(pool.base()), msg.sender, address(pool), a);

        // Execute the swap
        uint128 received = pool.sellBase(msg.sender, expected);

        emit Swap(u, m, u, address(pool.fyToken()), received, a, msg.sender);
        return received;
    }

function sellUnderlying(
    address u,
    uint256 m,
    uint128 a,
    uint128 s
) external returns (uint128) {
    // Get the pool for the market
    IPool pool = IPool(pools[u][m]);

    IERC20 token = IERC20(pool.base());

    // Transfer the underlying tokens to the pool
    Safe.transferFrom(token, msg.sender, address(pool), a);

    uint256 balanceBefore = token.balanceOf(msg.sender);

    // Execute the swap
    uint256 received = pool.sellBase(msg.sender, expected);

    uint256 balanceAfter = token.balanceOf(msg.sender);

    // may consider use safeCasting
    if(uint128(balanceAfter - received) < s) {
        revert Exception(16, expected, 0, address(0), address(0));
    }

    emit Swap(u, m, u, address(pool.fyToken()), received, a, msg.sender);
    return received;
}

sherlock-audit / 2022-10-illuminate-judging

ctf_sec - Slippage check should happens after the trade in MarketPlace.sol #95