xiaoming90 - Router can be DOSed by depositing 1 wei

[sherlock-admin2]

xiaoming90

medium

Router can be DOSed by depositing 1 wei

Summary

Router can be DOSed by depositing 1 wei, breaking the core protocol/contract functionality.

Vulnerability Detail

Many of the core functions depend on the _pay function. Note that at Line 100 below, if the token is WETH and the msg.value is non-zero, the TX will revert.

Thus, a malicious user can always deposit 1 wei of ETH to the Router to DOS the Router. As a result, users who want to interact with the Router via WETH tokens will not be able to do so. This attack is repeatable even after someone removed the 1 wei of ETH from the Router.

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/base/PeripheryPayments.sol#L100

File: PeripheryPayments.sol
077:     /// @dev Pay with token or WEH9 if the contract has enough ether
078:     /// @param token The token to pay
079:     /// @param payer The entity that must pay
080:     /// @param recipient The entity that will receive payment
081:     /// @param value The amount to pay
082:     function _pay(address token, address payer, address recipient, uint256 value) internal {
083:         if (token == address(WETH9) && address(this).balance >= value) {
084:             // pay with WETH9
085:             WETH9.deposit{value: value}(); // wrap only what is needed to pay
086:             WETH9.transfer(recipient, value);
087:         } else if (payer == address(this)) {
088:             IERC20(token).safeTransfer(recipient, value);
089:         } else {
090:             // pull payment
091: 
092:             // note: Check value sent to this contract is zero if token is WETH9
093:             // Corner case: A situation where the `msg.value` sent is not enough to satisfy `address(this).balance >= value`.
094:             // In such conditions, if we wouldn't revert, `IERC20(WETH).safeTransferFrom(payer, recipient, value)` will be executed,
095:             // and the `msg.value` will remain in the Router, potentially allowing the attacker to claim it.
096:             // This is why we ensure that the `msg.value` is zero for pulling WETH.
097: 
098:             // note: NapierRouter inherits from PeripheryPayments and Multicallable.
099:             // Basically, using `msg.value` in a loop can be dangerous but in this case, `msg.value` is not used for accounting purposes.
100:             if (token == address(WETH9) && msg.value > 0) revert Errors.RouterInconsistentWETHPayment();
101:             IERC20(token).safeTransferFrom(payer, recipient, value);
102:         }
103:     }

Impact

Router, which is one of the core features of the protocol, will be DOSed. Breaking of core protocol/contract functionality.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/base/PeripheryPayments.sol#L100

Tool used

Manual Review

Recommendation

In a situation where the msg.value sent is not enough to satisfy address(this).balance >= value, convert the existing msg.value sent to WETH, and pull the shortfall WETH balance from the payer account.

function _pay(address token, address payer, address recipient, uint256 value) internal {
    if (token == address(WETH9) && address(this).balance >= value) {
        // pay with WETH9
        WETH9.deposit{value: value}(); // wrap only what is needed to pay
        WETH9.transfer(recipient, value);
    } else if (payer == address(this)) {
        IERC20(token).safeTransfer(recipient, value);
    } else {
        // pull payment
        ..SNIP..
-       if (token == address(WETH9) && msg.value > 0) revert Errors.RouterInconsistentWETHPayment();
+       if (token == address(WETH9) && msg.value > 0) {
+           WETH9.deposit{value: msg.value}
+       }
-        IERC20(token).safeTransferFrom(payer, recipient, value);
+       IERC20(token).safeTransferFrom(payer, recipient, value - msg.value);
    }
}

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

According to sherlock docs this type of DoS is invalid.

[xiaoming9090]

Escalate.

The following core functions of the Router will stop working if this issue occurs:

• Add Liquidity - If LP cannot add assets to the pool, the pool will not have any liquidity, and nothing can function.

• Swap Underlying Asset for YT - If users cannot swap underlying assets for YT tokens, the market is effectively broken.

• Swap Underlying Asset for PT - If users cannot swap underlying assets for PT tokens, the market is effectively broken.

Per Sherlock's judging rule below, there are certain scenario where DOS related issues are considered valid Medium/High.

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue:

1. The issue causes locking of funds for users for more than a week.
2. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly. Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

For this issue, the attack can be repeated perpetually and cheaply (1 wei + gas fee), easily extending the attack for more than a week. When 1 wei has been deposited to the Router, the protocol team has to spend around an equal amount of gas fee to remove 1 wei from the Router contract to unlock the Router. In addition, the add liquidity and swaps of the Router are core functionality.

Note that it is not possible to bypass the Router contract by interacting with the Pool contract directly to workaround. The Pool contract has access controls to ensure that only the Router contract can call its functions.

[sherlock-admin2]

Escalate.

The following core functions of the Router will stop working if this issue occurs:

• Add Liquidity - If LP cannot add assets to the pool, the pool will not have any liquidity, and nothing can function.

• Swap Underlying Asset for YT - If users cannot swap underlying assets for YT tokens, the market is effectively broken.

• Swap Underlying Asset for PT - If users cannot swap underlying assets for PT tokens, the market is effectively broken.

Per Sherlock's judging rule below, there are certain scenario where DOS related issues are considered valid Medium/High.

Could Denial-of-Service (DOS), griefing, or locking of contracts count as a Medium (or High) issue? DoS has two separate scores on which it can become an issue:

1. The issue causes locking of funds for users for more than a week.
2. The issue impacts the availability of time-sensitive functions (cutoff functions are not considered time-sensitive). If at least one of these are describing the case, the issue can be a Medium. If both apply, the issue can be considered of High severity. Additional constraints related to the issue may decrease its severity accordingly. Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

For this issue, the attack can be repeated perpetually and cheaply (1 wei + gas fee), easily extending the attack for more than a week. When 1 wei has been deposited to the Router, the protocol team has to spend around an equal amount of gas fee to remove 1 wei from the Router contract to unlock the Router. In addition, the add liquidity and swaps of the Router are core functionality.

Note that it is not possible to bypass the Router contract by interacting with the Pool contract directly to workaround. The Pool contract has access controls to ensure that only the Router contract can call its functions.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cvetanovv]

The reason I haven't put it as a valid Medium is the Sherlock rules. That's why I think this issue is Low. Yes, a user can DoS this way, but it will be for a very short time, and will have to do it constantly. From the rules, "Additional constraints related to the issue may decrease its severity accordingly." That the protocol can fix this DoS relatively quickly and cheaply leads me to think this is Low rather than Medium.

[xiaoming9090]

Additional constraints related to the issue may decrease its severity accordingly

The DOS will only stop when the protocol team develops bots to constantly monitor the Router contract's balance and manually extract the 1 wei (if any) from the routers whenever a malicious actor performs this attack. One could also argue that the fact that this attack is extremely cheap and easy to carry out repeatedly increases the probability and impact of this issue, which leads to a higher severity rating.

[Czar102]

I'd like to consider this a valid Medium, nevertheless it seems to be a difficult task given the rules. I'll return to this finding.

[Czar102]

Unfortunately, I believe the judgment on this issue needs to stay as is based on the following rule:

Griefing for gas (…) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

The docs will be modified to better capture different cases of gas griefing.

Planning to reject the escalation and leave the issue as is.

[Czar102]

Result: Low Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: rejected