xiaoming90 - Tranche Router silently pulls WETH from users

[sherlock-admin]

xiaoming90

high

Tranche Router silently pulls WETH from users

Summary

Tranche Router will silently pull WETH from users if insufficient native ETH being sent to the contract, but did not refund back the unused native ETH at the end, leading to loss of assets for the affected users.

Vulnerability Detail

Assume that the underlyingAmount is 100 ETH, but the amount of native ETH sent to the contract is 99 ETH or any amount less than 100 ETH. Instead of reverting because of insufficient native ETH being sent to the contract, the code at Line 53 will silently be executed, attempting to pull 100 WETH from the user's account (It is common for a wallet to grant max allowance to Router in the past transactions).

The 100 WETH will be sent to the tranche to mint PY + YT. However, the issue is that the unused 99 ETH on the Router is not swept back to the user's account at the end of the TX. Thus, anyone else can obtain the 99 ETH that resides on the Router.

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/TrancheRouter.sol#L53

File: TrancheRouter.sol
35:     /// @notice deposit an `underlyingAmount` of underlying token into the yield source, receiving PT and YT.
36:     /// @dev Accept native ETH.
37:     /// @inheritdoc ITrancheRouter
38:     function issue(address adapter, uint256 maturity, uint256 underlyingAmount, address to)
39:         external
40:         payable
41:         nonReentrant
42:         returns (uint256)
43:     {
44:         ITranche tranche =
45:             TrancheAddress.computeAddress(adapter, maturity, TRANCHE_CREATION_HASH, address(trancheFactory));
46:         IERC20 underlying = IERC20(tranche.underlying());
47: 
48:         // Transfer underlying tokens to this contract
49:         // If this contract holds enough ETH, wrap it. Otherwise, transfer from the caller.
50:         if (address(underlying) == address(WETH9) && address(this).balance >= underlyingAmount) {
51:             WETH9.deposit{value: underlyingAmount}();
52:         } else {
53:             underlying.safeTransferFrom(msg.sender, address(this), underlyingAmount);
54:         }
55:         // Force approve
56:         underlying.forceApprove(address(tranche), underlyingAmount);
57: 
58:         return tranche.issue(to, underlyingAmount);
59:     }

Impact

Loss of assets for the affected users.

Code Snippet

https://github.com/sherlock-audit/2024-01-napier/blob/main/v1-pool/src/TrancheRouter.sol#L53

Tool used

Manual Review

Recommendation

Consider making the following changes so that it will not silently pull WETH from the users if insufficient ETH is sent.

function issue(address adapter, uint256 maturity, uint256 underlyingAmount, address to)
    external
    payable
    nonReentrant
    returns (uint256)
{
    ITranche tranche =
        TrancheAddress.computeAddress(adapter, maturity, TRANCHE_CREATION_HASH, address(trancheFactory));
    IERC20 underlying = IERC20(tranche.underlying());

    // Transfer underlying tokens to this contract
    // If this contract holds enough ETH, wrap it. Otherwise, transfer from the caller.
    if (address(underlying) == address(WETH9) && address(this).balance >= underlyingAmount) {
        WETH9.deposit{value: underlyingAmount}();
    } else {
+       if (token == address(WETH9) && msg.value > 0) revert Errors.RouterInconsistentWETHPayment();
        underlying.safeTransferFrom(msg.sender, address(this), underlyingAmount);
    }
    // Force approve
    underlying.forceApprove(address(tranche), underlyingAmount);

    return tranche.issue(to, underlyingAmount);
}

The above measure has already been implemented in the PeripheryPayments._pay function and the risk has been understood and documented in Line 95 below. However, this measure is not consistently applied to the other part of the protocol.

File: PeripheryPayments.sol
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:     }
104: }

[sherlock-admin]

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

tsvetanovv commented:

This is user input mistake

takarez commented:

valid: high(3)

[xiaoming9090]

Escalate.

In the report, the user intends to mint 100 ETH worth of PT/YT. Although this is a user mistake, as the user should be sending 100 Native ETH instead of 99 Native ETH, the code should revert instead of silently pulling another 100 WETH from the user's wallet address when the protocol receives insufficient ETH. So, there is a logic error here.

This results in the protocol receiving 199 ETH from the users while only using 100 ETH for the mint process, leaving behind 99 ETH sitting in the Router contract that anyone can steal. As a result, the user lost 99 ETH, a significant loss of funds in this example.

Note that the victim would not have known that their 99 ETH is sitting in the Router contract because the extra ETH is silently pulled from their wallet. By the time the victim is aware of the reduced ETH in their wallet, it is already too late. MEV and bots already retrieved the remaining ETH on the Router immediately after Bob's transaction.

Sherlock's judging rule below states that issues related to users' mistakes can be accepted if they could lead to a significant loss of funds. Thus, this issue is valid and justifiable as a High issue.

User input validation: User input validation to prevent user mistakes is not considered a valid issue. However, if a user input could result in a major protocol malfunction or significant loss of funds could be a valid high

Taken from https://docs.sherlock.xyz/audits/judging/judging

[sherlock-admin2]

Escalate.

In the report, the user intends to mint 100 ETH worth of PT/YT. Although this is a user mistake, as the user should be sending 100 Native ETH instead of 99 Native ETH, the code should revert instead of silently pulling another 100 WETH from the user's wallet address when the protocol receives insufficient ETH. So, there is a logic error here.

This results in the protocol receiving 199 ETH from the users while only using 100 ETH for the mint process, leaving behind 99 ETH sitting in the Router contract that anyone can steal. As a result, the user lost 99 ETH, a significant loss of funds in this example.

Note that the victim would not have known that their 99 ETH is sitting in the Router contract because the extra ETH is silently pulled from their wallet. By the time the victim is aware of the reduced ETH in their wallet, it is already too late. MEV and bots already retrieved the remaining ETH on the Router immediately after Bob's transaction.

Sherlock's judging rule below states that issues related to users' mistakes can be accepted if they could lead to a significant loss of funds. Thus, this issue is valid and justifiable as a High issue.

User input validation: User input validation to prevent user mistakes is not considered a valid issue. However, if a user input could result in a major protocol malfunction or significant loss of funds could be a valid high

Taken from https://docs.sherlock.xyz/audits/judging/judging

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.

[nevillehuang]

@xiaoming9090 There is no malfunction of protocol here. This should remain invalid.

[xiaoming9090]

@xiaoming9090 There is no malfunction of protocol here. This should remain invalid.

This report and my earlier response above have clearly demonstrated the loss of funds for the users. Thus, it should be valid.

[cvetanovv]

I disagree with the escalation. Still seems like a user input mistake to me.

[xiaoming9090]

Sherlock's judging rule below states that issues related to users' mistakes can be accepted if they could lead to a significant loss of funds. Thus, this issue is valid and justifiable as a High issue.

User input validation: User input validation to prevent user mistakes is not considered a valid issue. However, if a user input could result in a major protocol malfunction or significant loss of funds could be a valid high Taken from https://docs.sherlock.xyz/audits/judging/judging

A mistake in user input does not necessarily mean the issue is invalid in Sherlock. In this case, it is a combination of a user's mistake and logic error in the implementation, which led to a loss of funds. Also, Sherlock's judging rule below states that issues related to users' mistakes can be accepted up to High if they could lead to a loss of funds.

User input validation: User input validation to prevent user mistakes is not considered a valid issue. However, if a user input could result in a major protocol malfunction or significant loss of funds could be a valid high

Taken from https://docs.sherlock.xyz/audits/judging/judging

[Czar102]

The rule quoted concerns a loss/protocol malfunction on a higher level than for a user. I will make the above rule clearer.

Nevertheless, this is a user mistake and not a valid issue. Planning to reject the escalation and leave the issue as is.

[Czar102]

Result: Invalid Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: rejected