y4y - If deposit token is also reward token, deposit cap may have unexpected behaviors

[sherlock-admin2]

y4y

Medium

If deposit token is also reward token, deposit cap may have unexpected behaviors

Summary

In the case of deposited token is also reward token, it would unexpectingly exceed the cap amount, despite the actual deposited amount is less than capped amount.

Root Cause

In PTokenVault::deposit, we see that there is a capped value for the amount user can deposit in:

    function deposit(ERC20 _token, uint256 _amount, address _receiver) public {
        uint256 cap = caps[address(_token)];

        if (cap != type(uint256).max) {
            if (_amount + _token.balanceOf(address(this)) > cap) {
                revert DepositExceedsCap();
            }
        }

        _token.safeTransferFrom(msg.sender, address(this), _amount);

        balances[_receiver][_token] += _amount;

        emit Deposit(msg.sender, _receiver, address(_token), _amount);
    }

which the limit is checked by comparing the sum of deposited amount and current token balance to the capped value. The vault also offers users to convert the reward tokens to PTokens, this essentially transfers reward token to the vault, and the vault calculate correspond amount of PTokens user would get in return:

    function convertRewardsToPTokens(address _receiver, bytes32 _pointsId, uint256 _amountToConvert) public {
        RedemptionParams memory params = redemptions[_pointsId];
        (ERC20 rewardToken, uint256 rewardsPerPToken, bool isMerkleBased) =
            (params.rewardToken, params.rewardsPerPToken, params.isMerkleBased);

        if (address(rewardToken) == address(0)) {
            revert RewardsNotReleased();
        }

        if (isMerkleBased) {
            revert CantConvertMerkleRedemption();
        }

        rewardToken.safeTransferFrom(msg.sender, address(this), _amountToConvert);

        uint256 pTokensToMint = FixedPointMathLib.divWadDown(_amountToConvert, rewardsPerPToken); // Round down for mint.

        // Dust guard.
        if (pTokensToMint == 0) {
            revert AmountTooSmall();
        }

        pTokens[_pointsId].mint(_receiver, pTokensToMint);

        emit RewardsConverted(msg.sender, _receiver, _pointsId, _amountToConvert);
    }

The issue here is that, the deposited token can also be the reward token, and in this case, when users start to convert this token to PTokens, it would make the balance of vault increase, but this value increase is not caused by depositing.

Internal pre-conditions

The vault allows deposits of USDC, and it also use USDC as one of the reward tokens. The deposit cap of USDC is set to 5000, for simplicity.

External pre-conditions

After redemption period has started, users can convert some USDC tokens into PTokens as USDC is one of the reward tokens. This would increase ERC20(USDC).balanceOf(address(this)), the balance quickly reaches 6000, with 4000 being user deposits, and the rest 2000 is converted tokens.

Attack Path

No response

Impact

Combined with both internal, and external pre-conditions, though users have deposited in total 4000 USDC, which is still 1000 less from reaching the capped value, due to this additional 2000 converted amount, deposit would revert, as the account balance exceeds the capped value. This is unexpected for depositors, and according to a PT with the protocols, the capped amount is indeed meant to cap the total deposited tokens. Conversion of PTokens provides user incentives to do so, so unlike simply donating to the contract, the chance of this happening is larger.

The admins can always increase the capped value, but for some tokens, they may want to keep the amount low, and when the conditions are all met, this would break the invariant of the protocol.

PoC

No response

Mitigation

Use a separate variable to store total deposited amount for each token

[Alireza-Razavi]

This seems to be low due to sponsor's comment:

while this is accurate, it does not meet the criteria for a medium. The cap of the token can always be raised by the admin, which means the contract is not rendered useless, and there is no loss of funds in this scenario.

[brandonshiyay]

Escalate
While I do agree that the admins can always increase the cap on this, it still opens a small window for DoS to happen. Since the comparison is based on token balance of the contract, under normal scenarios, it would make no sense to launch a donation type of attack as there is no incentive and attacker earns nothing. In the scenario I have described in my report, which the deposit token and the reward token being exactly the same, actually gives anyone including an attacker to convert the token into PTokens, as they can trade, exchange, or eventually redeem their PTokens back to reward tokens.

Let's suppose this, token A is set to be one of the deposit token and reward token at the same time, and the protocol team wants to limit the deposit cap at 1000e18, meaning users can at most deposit 1000e18 token A into the vault. At the same time, as token A being also a reward token, users may convert token A into PTokens, as they can trade or redeem later. This, for example, makes the current balance of the vault in token A reach 400e18, and only 600e18 is actually available for depositors to deposit. Admins observe this, and reset the cap to a higher amount, 1400e18, as a counter measure for this situiation. Right before or after this transaction, an attacker can convert another 800e18 token A to the vault for PTokens, making the current balance 1200e18, and only 200e18 available left for depositors. This would again, require admins to increase the cap so that more people can deposit to reach the cap amount. But this would also allow an attacker to convert more token A, increasing the balance again, not alone other users may also convert token A as well.

This would cause some degrees of DoS, the reward conversion brings the attacker and other users an incentive to continue do so, and in the end, the attacker can just redeem his previously converted tokens back, all he loses will be some fees for redeeming, and it can block one of the major feature of the vault for a good while. Additionally, after the large amount redemption, as the balance decreases, the cap needs to be reset again to match the current level of balance, this also can make some users to frontrun the cap reset transaction to deposit more than the protocol would wanted.

All I want to express is that in the scenaior I have described, it's very likely to break the vault's invariant. As the balance change is dynamic, it would be quite unoptimistic for the admins to reset cap over and over again whenever they see some user converts reward token, or redeem in other scenario. All of those reasonings I provided above, I believe this should be a valid medium.

[sherlock-admin3]

Escalate
While I do agree that the admins can always increase the cap on this, it still opens a small window for DoS to happen. Since the comparison is based on token balance of the contract, under normal scenarios, it would make no sense to launch a donation type of attack as there is no incentive and attacker earns nothing. In the scenario I have described in my report, which the deposit token and the reward token being exactly the same, actually gives anyone including an attacker to convert the token into PTokens, as they can trade, exchange, or eventually redeem their PTokens back to reward tokens.

Let's suppose this, token A is set to be one of the deposit token and reward token at the same time, and the protocol team wants to limit the deposit cap at 1000e18, meaning users can at most deposit 1000e18 token A into the vault. At the same time, as token A being also a reward token, users may convert token A into PTokens, as they can trade or redeem later. This, for example, makes the current balance of the vault in token A reach 400e18, and only 600e18 is actually available for depositors to deposit. Admins observe this, and reset the cap to a higher amount, 1400e18, as a counter measure for this situiation. Right before or after this transaction, an attacker can convert another 800e18 token A to the vault for PTokens, making the current balance 1200e18, and only 200e18 available left for depositors. This would again, require admins to increase the cap so that more people can deposit to reach the cap amount. But this would also allow an attacker to convert more token A, increasing the balance again, not alone other users may also convert token A as well.

This would cause some degrees of DoS, the reward conversion brings the attacker and other users an incentive to continue do so, and in the end, the attacker can just redeem his previously converted tokens back, all he loses will be some fees for redeeming, and it can block one of the major feature of the vault for a good while. Additionally, after the large amount redemption, as the balance decreases, the cap needs to be reset again to match the current level of balance, this also can make some users to frontrun the cap reset transaction to deposit more than the protocol would wanted.

All I want to express is that in the scenaior I have described, it's very likely to break the vault's invariant. As the balance change is dynamic, it would be quite unoptimistic for the admins to reset cap over and over again whenever they see some user converts reward token, or redeem in other scenario. All of those reasonings I provided above, I believe this should be a valid medium.

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.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
sense-finance/point-tokenization-vault#38

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.

[WangSecurity]

But this would also allow an attacker to convert more token A, increasing the balance again, not alone other users may also convert token A as well.

But, the attacker converting more tokens is not an issue.

This would cause some degrees of DoS, the reward conversion brings the attacker and other users an incentive to continue do so, and in the end, the attacker can just redeem his previously converted tokens back, all he loses will be some fees for redeeming, and it can block one of the major feature of the vault for a good while.

I don't think it can be called a DOS. Let's look at the scenario without an attacker, the users just deposit into the vault, the cap is 5000 USDC and users have deposited 4999 USDC. No one can deposit more than 1 USDC, but it's not a DOS, it's the exact design of the vault and the cap.

Moreover, as said above, the admin can always set the cap higher and there's no DOS of time-sensitive functions (deposit is not time-sensitive) or a lock of funds for longer than 7-days.

Hence, I believe the lead judge was correct here and this issue issue has to remain invalid, planning to reject the escalation.

[brandonshiyay]

Thanks for your reply, but I have some more comment on this.

First, I want to say that the scenario I have been describing is when the deposit token is also a reward token. As noted in my report, that the deposit function has a cap to it, meaning that the total deposited amount from user cannot exceed this value, for example, if usdc is such token, and the cap is 5000, the total amount users can deposit should not go pass 5000.

Like you commented, when the cap is 5000, and the current balance is 4999, user cannot deposit more than 1, as this is per designed. However, when the deposit token, usdc, is also a reward token, a new factor is introduced. Note that there is another feature of vault, which allows users to convert a reward token to PToken, and this is not related to deposit function. When an user who does not want to deposit, only wants to convert, he can supply some amount of usdc, and transfer those to the vault, say 1000. Now, the balance of vault on usdc is 1000, but let's assume no one has deosited yet, so the deposited amount is 0. But in reality, when a depositor comes in, and see that the cap is 5000, so he wants to deposit 4500, as it's below the cap, and the total deposited amount would not pass capped 5000, and he would fail to do so, as the total balance is used to do the check, and makes the value to be 5500, which is over 5000, hence revert.

The design is to let user deposit 5000 in total, but due to this faulty logic, users can only deposit 4000. Sure, admins can always increase the cap, but they do not have control over other users who want to convert reward tokens. The invariant is this capped value, but in the scenario I proposed, this invariant has a broken state.

If we bring in an external malicious actor, he can also convert such reward token, increasing the balance of such token, but without having to deposit. This will break the invariant, and as admins try to increase the cap, this actor can always make a counter move, either by redeeming or convert more. The result is that, the designed feature of 'total amount of deposit allowed' does not work, and making users to either deposit more or less than this enforced amount. Hencr I consider this to be a somewhat DOS.

In short, when reward token is also deposit token, this capped design cannot be enforced, even with admins trying to reset cap value over and over again. This is because converting reward tokens is not related to deposit, and the amount converted should have no interference on the total amount users allowed to deposit. I hope my comment will make things clear.

[WangSecurity]

Thank you for elaborating on your issue. I think the confusion between us was due to my phrasing. With my example of 4999 USDC out of 5000, I meant to say that in this case it's not a DOS, even though the users cannot deposit. I understand your issue is different.

However, the admin can partially mitigate this issue by increasing the cap, so deposits are live again. Moreover, it's the TRUSTED role (operator) that sets the reward token. There are no limitations on admin-set variables, hence, we should assume the admins will use the reward token that won't cause any issues:

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.
If no values are provided, the (external) admin is trusted to use values that will not cause any issues.
Note and Exception are omitted since they are irrelevant

Hence, my decision remains based on both of the above arguments, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• brandonshiyay: rejected