Anyone can steal excess ETH from users

Summary

The PeripheryPayments contract ideally shouldn't hold funds, however, because excess tokens are not refunded to users upon swaps, anyone can frontrun the calls to the refund functions to steal the user's excess tokens.

Vulnerability Detail

The PeripheryPayments contract has its functions public without access control for the most part, so anyone can call the functions.

Using the swapUnderlyingForPt function as an example, users can call the function to swap tokens orETH for principal tokens. Situations can arise in which the amount sent in for the swap will have leftover e.g partial swaps, extremely efficient swaps, causing some tokens unspent (even when it was pre-computed precisely by the caller) and so on.

For this case, the tokens are not automatically refunded back to the user. He has to manually call the refundETH function to get the remaining ether.

Since the refundETH function is accesible to anyone, the user's call to the function can be frontrun by malicious attacker monitoring the mempool and can be used to steal the user's refunds.

Impact

Loss of user's excess ether.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/NapierRouter.sol#L239

    function swapUnderlyingForPt(
        address pool,
        uint256 index,
        uint256 ptOutDesired,
        uint256 underlyingInMax,
        address recipient,
        uint256 deadline
    ) external payable override nonReentrant checkDeadline(deadline) returns (uint256) {
        // dev: Optimistically call to the `pool` provided by the untrusted caller.
        // And then verify the pool using CREATE2.
        (address underlying, address basePool) = INapierPool(pool).getAssets();

        // if `pool` doesn't matched, it would be reverted.
        if (INapierPool(pool) != PoolAddress.computeAddress(basePool, underlying, POOL_CREATION_HASH, address(factory)))
        {
            revert Errors.RouterPoolNotFound();
        }

        IERC20 pt = INapierPool(pool).principalTokens()[index];

        // Abi encode callback data to be used in swapCallback
        bytes memory data = new bytes(0xa0);
        {
            uint256 callbackType = uint256(CallbackType.SwapUnderlyingForPt);
            assembly {
                // Equivanlent to:
                // data = abi.encode(CallbackType.SwapUnderlyingForPt, underlying, basePool, CallbackDataTypes.SwapUnderlyingForPtData({payer: msg.sender, underlyingInMax: underlyingInMax}))
                mstore(add(data, 0x20), callbackType)
                mstore(add(data, 0x40), underlying)
                mstore(add(data, 0x60), basePool)
                mstore(add(data, 0x80), caller()) // dev: Ensure 'payer' is always 'msg.sender' to prevent allowance theft on callback.
                mstore(add(data, 0xa0), underlyingInMax)
            }
        }

        uint256 prevBalance = pt.balanceOf(address(this));
        uint256 underlyingUsed = INapierPool(pool).swapUnderlyingForPt(
            index,
            ptOutDesired,
            address(this), // this contract will receive principal token from pool
            data
        );

        pt.safeTransfer(recipient, pt.balanceOf(address(this)) - prevBalance);
        return underlyingUsed;
    }

https://github.com/sherlock-audit/2024-01-napier/blob/6313f34110b0d12677b389f0ecb3197038211e12/v1-pool/src/base/PeripheryPayments.sol#L67-L70


    function refundETH() external payable {
        if (address(this).balance > 0) _safeTransferETH(msg.sender, address(this).balance);
    }

Tool used

Manual Code Review

Recommendation

Reconsider the decision to allow users to manually call refundETH and execute refunds automatically after swaps.

Duplicate of #81

sherlock-audit / 2024-01-napier-judging

ZanyBonzy - Anyone can steal excess ETH from users #22