Certain function should not be marked as payable in Router.sol

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/libraries/SelfPermit.sol#L16 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/libraries/SelfPermit.sol#L21 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/libraries/SelfPermit.sol#L26 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/libraries/SelfPermit.sol#L31 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L59 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L70 https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/router-v1/contracts/Router.sol#L80

Vulnerability details

Impact

Certain function should not be marked as payable, otherwise if user accidentally attach ETH in the function call, the ETH is lost.

Proof of Concept

There are a few function that should not be marked payable

The Router contract inherits from SelfPermit.

contract Router is IRouter, Multicall, SelfPermit, Deadline {

all function is SelfPermit should not be payable:

abstract contract SelfPermit is ISelfPermit {
    /// @inheritdoc ISelfPermit
    function selfPermit(address token, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) public payable override {
        IERC20Permit(token).permit(msg.sender, address(this), value, deadline, v, r, s);
    }

    /// @inheritdoc ISelfPermit
    function selfPermitIfNecessary(address token, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) external payable override {
        if (IERC20(token).allowance(msg.sender, address(this)) < value) selfPermit(token, value, deadline, v, r, s);
    }

    /// @inheritdoc ISelfPermit
    function selfPermitAllowed(address token, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) public payable override {
        IERC20PermitAllowed(token).permit(msg.sender, address(this), nonce, expiry, true, v, r, s);
    }

    /// @inheritdoc ISelfPermit
    function selfPermitAllowedIfNecessary(address token, uint256 nonce, uint256 expiry, uint8 v, bytes32 r, bytes32 s) external payable override {
        if (IERC20(token).allowance(msg.sender, address(this)) < type(uint256).max) selfPermitAllowed(token, nonce, expiry, v, r, s);
    }
}

the function in the SelfPermit contract let user sign a signature offchain and use the signature to take allowance, the only purpose is to set allowance, so these functions should not be marked as payable.

In Router.sol, the function unwrapWETH9 and sweepToken and refundETH should not be marked as payable because these function are only desigend to resuce token stucked in the router.

/// @inheritdoc IRouter
function unwrapWETH9(uint256 amountMinimum, address recipient) public payable override {
    uint256 balanceWETH9 = WETH9.balanceOf(address(this));
    require(balanceWETH9 >= amountMinimum, "Insufficient WETH9");

    if (balanceWETH9 > 0) {
        WETH9.withdraw(balanceWETH9);
        TransferHelper.safeTransferETH(recipient, balanceWETH9);
    }
}

/// @inheritdoc IRouter
function sweepToken(IERC20 token, uint256 amountMinimum, address recipient) public payable {
    uint256 balanceToken = token.balanceOf(address(this));
    require(balanceToken >= amountMinimum, "Insufficient token");

    if (balanceToken > 0) {
        TransferHelper.safeTransfer(address(token), recipient, balanceToken);
    }
}

/// @inheritdoc IRouter
function refundETH() external payable override {
    if (address(this).balance > 0) TransferHelper.safeTransferETH(msg.sender, address(this).balance);
}

for addLiqudity related function in Router, the function should wrap the ETH to WETH for msg.sender, it is just two ERC20 token pair, the function should not be marked as payable.

given that the code does not refund any excessive ETH sent to the contract,

Anyone can call refundETH to sweep ETH for free.

/// @inheritdoc IRouter
function refundETH() external payable override {
    if (address(this).balance > 0) TransferHelper.safeTransferETH(msg.sender, address(this).balance);
}

Then if user accidentally attach ETH in the function call to payable function that should not be marked as payable, the ETH is lost.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the project remove payable keywords for functions that do not need the payable keywords and refund excessive ETH.

[hansfriese]

The refundETH() is exposed with that case in mind, isn't it? If a user sends ETH by mistake, he can call refundETH() to get it back.

[gte620v]

There are a few issues raised here: 1) There are unnecessary payables. This is a gas optimization. https://medium.com/coinmonks/deciphering-solidity-does-adding-a-payable-keyword-actually-save-gas-89d1d8298d3f 2) "Anyone can call refundETH to sweep ETH for free". This is the intended behavior and is a dup of https://github.com/code-423n4/2022-12-Stealth-Project-findings/issues/30

[c4-sponsor]

gte620v marked the issue as sponsor disputed

[c4-sponsor]

gte620v marked the issue as disagree with severity

[kirk-baird]

I disagree with the severity as these are deliberate gas optimisations and require significant user error to exploit.

However, I do agree with the warden that the gas optimisation is not worth the security risk of users accidentally sending funds. I will therefore consider this QA.

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-12-Stealth-Project-findings/issues/62