0xadrii - Not properly tracking debt accrual leads mintOpenInterestDebt() to lose twTap rewards

[sherlock-admin3]

0xadrii

medium

Not properly tracking debt accrual leads mintOpenInterestDebt() to lose twTap rewards

Summary

Debt accrual is tracked wrongly, making the expected twTap rewards to be potentially lost.

Vulnerability Detail

Penrose’s mintOpenInterestDebt() function allows USDO to be minted and distributed as a reward to twTap holders based on the current USDO open interest.

In order to mint and distribute rewards, mintOpenInterestDebt() will perform the following steps:

1. Query the current USDO.supply()
2. Compute the total debt from all the markets (Origins included)
3. If totalUsdoDebt > usdoSupply, then distribute the difference among the twTap holders

function mintOpenInterestDebt(address twTap) external onlyOwner { 
        uint256 usdoSupply = usdoToken.totalSupply();

        // nothing to mint when there's no activity
        if (usdoSupply > 0) {  
            // re-compute latest debt
            uint256 totalUsdoDebt = computeTotalDebt();  

            //add Origins debt 
            //Origins market doesn't accrue in time but increases totalSupply
            //and needs to be taken into account here
            uint256 len = allOriginsMarkets.length;
            for (uint256 i; i < len; i++) {
                IMarket market = IMarket(allOriginsMarkets[i]);
                if (isOriginRegistered[address(market)]) {
                    (uint256 elastic,) = market.totalBorrow();
                    totalUsdoDebt += elastic;
                }
            }

            //debt should always be > USDO supply
            if (totalUsdoDebt > usdoSupply) { 
                uint256 _amount = totalUsdoDebt - usdoSupply;

                //mint against the open interest; supply should be fully minted now
                IUsdo(address(usdoToken)).mint(address(this), _amount);

                //send it to twTap
                uint256 rewardTokenId = ITwTap(twTap).rewardTokenIndex(address(usdoToken));
                _distributeOnTwTap(_amount, rewardTokenId, address(usdoToken), ITwTap(twTap));
            }
        } 
    }

This approach has two main issues that make the current reward distribution malfunction:

1. Because debt is not actually tracked and is instead directly queried from the current total borrows via computeTotalDebt(), if users repay their debt prior to a reward distribution this debt won’t be considered for the fees, given that fees will always be calculated considering the current totalUsdoDebt and usdoSupply.
2. Bridging USDO is not considered
   1. If USDO is bridged from another chain to the current chain, then the usdoToken.totalSupply() will increment but the totalUsdoDebt() won’t. This will make rewards never be distributed because usdoSupply will always be greater than totalUsdoDebt.
   2. On the other hand, if USDO is bridged from the current chain to another chain, the usdoToken.totalSupply() will decrement and tokens will be burnt, while totalUsdoDebt() will remain the same. This will make more rewards than the expected ones to be distributed because usdoSupply will be way smaller than totalUsdoDebt.

Proof of concept

Consider the following scenario: 1000 USDO are borrowed, and already 50 USDO have been accrued as debt.

This makes USDO’s totalSupply() to be 1000, while totalUsdoDebt be 1050 USDO. If mintOpenInterestDebt() is called, 50 USDO should be minted and distributed among twTap holders.

However, prior to executing mintOpenInterestDebt(), a user bridges 100 USDO from chain B, making the total supply increment from 1000 USDO to 1100 USDO. Now, totalSupply() is 1100 USDO, while totalUsdoDebt is still 1050, making rewards not be distributed among users because totalUsdoDebt < usdoSupply.

Impact

Medium. The fees to be distributed in twTap are likely to always be wrong, making one of the core governance functionalities (locking TAP in order to participate in Tapioca’s governance) be broken given that fee distributions (and thus the incentives to participate in governance) won’t be correct.

Code Snippet

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/Tapioca-bar/contracts/Penrose.sol#L263-L295

Tool used

Manual Review

Recommendation

One of the possible fixes for this issue is to track debt with a storage variable. Every time a repay is performed, the difference between elastic and base could be accrued to the variable, and such variable could be decremented when the fees distributions are performed. This makes it easier to compute the actual rewards and mitigates the cross-chain issue.

[sherlock-admin2]

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

takarez commented:

valid; medium(3)

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/Tapioca-DAO/Tapioca-bar/pull/380; https://github.com/Tapioca-DAO/tapioca-periph/pull/203.

[0xadrii]

Escalate

Although this issue was initially marked as Medium, I believe it should actually be set as High severity.

As shown in the report, this bug has two main issues:

1. Rewards can be lost due to users repaying prior to reward distribution
2. Bridging USDO is not considered, which might have two possible outcomes that affect the protocol:
   • Bridge USDO from another chain to the chain where rewards are being distributed, thus incrementing USDO's usdoSupply, and effectively preventing rewards being distributed due to usdoSupply being greater than totalUsdoDebt (this can be considered as medium impact, as this is not a direct loss of funds and is rather missing a portion of expected rewards)
   • Prior to reward distribution, bridge USDO from the chain where rewards are being distributed to another chain. This is the actual scenario that will have a high impact to the protocol. Below is a detailed explanation focusing on this exact scenario.

As per the code implementation, the difference between totalUsdoDebt debt compared with the current usdoSupply will be minted as twTap rewards:

// Penrose.sol
...
//debt should always be > USDO supply
 function mintOpenInterestDebt(address twTap) external onlyOwner { 
    if (totalUsdoDebt > usdoSupply) { 
        uint256 _amount = totalUsdoDebt - usdoSupply;

        //mint against the open interest; supply should be fully minted now
        IUsdo(address(usdoToken)).mint(address(this), _amount);

        //send it to twTap
        uint256 rewardTokenId = ITwTap(twTap).rewardTokenIndex(address(usdoToken));
        _distributeOnTwTap(_amount, rewardTokenId, address(usdoToken), ITwTap(twTap));
    }
 }

The high impact attack vector where USDO bridging is not considered allows an attacker to bridge USDO right before rewards are about to be distributed. This will make usdoSupply decrease, making the _amount obtained from substracting usdoSupply to totalUsdoDebt be actually bigger than what it should be.

In order to better understand the impact of the attack, consider the following scenario:

• Currently the protocol has a totalUsdoDebt of 1500 USDO and a usdoSupply of 1000 USDO. This means that a reward distribution would mint 500 USDO (totalUsdoDebt - usdoSupply) to be distributed on twTap.

1. An attacker borrows 1000 USDO, thus increasing totalUsdoDebt to 2500 USDO, and usdoSupply to 2000 USDO. If a reward distribution was to take place now, the amount of USDO to be distributed would still be 500 USDO, because the borrow made totalUsdoDebt and usdoSupply increase at the same time.
2. The attacker then decides to bridge their 1000 USDO to another chain. This will make usdoSupply in the current chain decrease and become 1000 USDO again. However, totalUsdoDebt is still 2500 USDO because totalUsdoDebt is obtained from the computeTotalDebt() function, which as mentioned in my report fetches the debt data from each market's totalBorrow variable (a variable that does NOT get modified when a bridge takes place)
3. The protocol team decides to execute a reward distribution by calling mintOpenInterestDebt(). Although the actual amount that should be minted and distributed is 500 USDO, the real amount that will be minted is 1500 USDO (2500 USDO of totalUsdoDebt - 1000 USDO of usdoSupply). This makes 1000 more USDO to be minted than the intended.

As mentioned, the severity of this attack should be considered as high because:

• An important loss of funds can be produced. An attacker can perform this attack every time a reward distribution takes place (which, as mentioned in the docs, is expected to be performed in weekly epochs), and the attacker is not heavily constrained to perform the attack.
• It breaks the core mechanism of the protocol of keeping USDO peg. Because this attack makes the USDO supply be way greater than what is intended, the excess of supply will affect USDO's peg, which should be considered as a high impact for a protocol that plans to release a stablecoin.

[sherlock-admin2]

Escalate

Although this issue was initially marked as Medium, I believe it should actually be set as High severity.

As shown in the report, this bug has two main issues:

1. Rewards can be lost due to users repaying prior to reward distribution
2. Bridging USDO is not considered, which might have two possible outcomes that affect the protocol:
   • Bridge USDO from another chain to the chain where rewards are being distributed, thus incrementing USDO's usdoSupply, and effectively preventing rewards being distributed due to usdoSupply being greater than totalUsdoDebt (this can be considered as medium impact, as this is not a direct loss of funds and is rather missing a portion of expected rewards)
   • Prior to reward distribution, bridge USDO from the chain where rewards are being distributed to another chain. This is the actual scenario that will have a high impact to the protocol. Below is a detailed explanation focusing on this exact scenario.

As per the code implementation, the difference between totalUsdoDebt debt compared with the current usdoSupply will be minted as twTap rewards:

// Penrose.sol
...
//debt should always be > USDO supply
 function mintOpenInterestDebt(address twTap) external onlyOwner { 
    if (totalUsdoDebt > usdoSupply) { 
        uint256 _amount = totalUsdoDebt - usdoSupply;

        //mint against the open interest; supply should be fully minted now
        IUsdo(address(usdoToken)).mint(address(this), _amount);

        //send it to twTap
        uint256 rewardTokenId = ITwTap(twTap).rewardTokenIndex(address(usdoToken));
        _distributeOnTwTap(_amount, rewardTokenId, address(usdoToken), ITwTap(twTap));
    }
 }

The high impact attack vector where USDO bridging is not considered allows an attacker to bridge USDO right before rewards are about to be distributed. This will make usdoSupply decrease, making the _amount obtained from substracting usdoSupply to totalUsdoDebt be actually bigger than what it should be.

In order to better understand the impact of the attack, consider the following scenario:

• Currently the protocol has a totalUsdoDebt of 1500 USDO and a usdoSupply of 1000 USDO. This means that a reward distribution would mint 500 USDO (totalUsdoDebt - usdoSupply) to be distributed on twTap.

1. An attacker borrows 1000 USDO, thus increasing totalUsdoDebt to 2500 USDO, and usdoSupply to 2000 USDO. If a reward distribution was to take place now, the amount of USDO to be distributed would still be 500 USDO, because the borrow made totalUsdoDebt and usdoSupply increase at the same time.
2. The attacker then decides to bridge their 1000 USDO to another chain. This will make usdoSupply in the current chain decrease and become 1000 USDO again. However, totalUsdoDebt is still 2500 USDO because totalUsdoDebt is obtained from the computeTotalDebt() function, which as mentioned in my report fetches the debt data from each market's totalBorrow variable (a variable that does NOT get modified when a bridge takes place)
3. The protocol team decides to execute a reward distribution by calling mintOpenInterestDebt(). Although the actual amount that should be minted and distributed is 500 USDO, the real amount that will be minted is 1500 USDO (2500 USDO of totalUsdoDebt - 1000 USDO of usdoSupply). This makes 1000 more USDO to be minted than the intended.

As mentioned, the severity of this attack should be considered as high because:

• An important loss of funds can be produced. An attacker can perform this attack every time a reward distribution takes place (which, as mentioned in the docs, is expected to be performed in weekly epochs), and the attacker is not heavily constrained to perform the attack.
• It breaks the core mechanism of the protocol of keeping USDO peg. Because this attack makes the USDO supply be way greater than what is intended, the excess of supply will affect USDO's peg, which should be considered as a high impact for a protocol that plans to release a stablecoin.

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]

This issue should remain Medium. You have escalated the report to change from Medium to High, with an additional attack vector that is not described in the main report. When deciding on a severity the main report is looked at, escalations are if a report is not judged correctly. With this escalation, you boost the report with a lot of additions already after the contest is over. I don't know if this is according to Sherlock rules, and if it will be a problem for future decisions when someone can put Medium then read the other reports and get a context to increase his severity to High.

But even if we don't consider what I wrote above, I think it should remain Medium. While the attack is valid the material losses are limited and the attack will only be valid when rewards are about to be distributed. To be a valid High according to Sherlock's rules: "Definite loss of funds without (extensive) limitations of external conditions."

[cvetanovv]

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

[Evert0x]

Result: Medium Unique

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• 0xadrii: rejected