ctf_sec - Share computing for reward distribution is incorrect

[sherlock-admin4]

ctf_sec

medium

Share computing for reward distribution is incorrect

Summary

Share computing for reward distribution is incorrect

Vulnerability Detail

Line of code

function _depositFeesToTwTap(IMarket market, ITwTap twTap) private {
        if (!isMarketRegistered[address(market)]) revert NotValid();

        uint256 feeShares = market.refreshPenroseFees();
        if (feeShares == 0) return;

        address _asset = market.asset();
        uint256 _assetId = market.assetId();

        yieldBox.withdraw(_assetId, address(this), address(this), 0, feeShares);

        uint256 rewardTokenId = twTap.rewardTokenIndex(_asset);
        uint256 feeAmount = yieldBox.toAmount(_assetId, feeShares, false);
        _distributeOnTwTap(feeAmount, rewardTokenId, _asset, twTap);
    }

This code in Penrose.sol is used to distribute the reward to twTAP.sol user that lock their TAP token

but the problem is that we are calling

   yieldBox.withdraw(_assetId, address(this), address(this), 0, feeShares);

and ignored the return parameter:

Line of code

function withdraw(
        uint256 assetId,
        address from,
        address to,
        uint256 amount,
        uint256 share
    ) external returns (uint256 amountOut, uint256 shareOut);

the returned amountOut should be what used for reward distribution

but the code compute the feeAmount again using stale shares

     uint256 feeAmount = yieldBox.toAmount(_assetId, feeShares, false);
     _distributeOnTwTap(feeAmount, rewardTokenId, _asset, twTap);

if feeAmount is greater than amountOut, calling _distributeOnTwTap will revert and block reward distribution.

Line of code

function _distributeOnTwTap(uint256 amount, uint256 rewardTokenId, address _asset, ITwTap twTap) private {
        _asset.safeApprove(address(twTap), amount);
        twTap.distributeReward(rewardTokenId, amount);
        emit LogTwTapFeesDeposit(amount);
    }

Impact

.

Code Snippet

1. https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/Tapioca-bar/contracts/Penrose.sol#L565

2. https://github.com/Tapioca-DAO/tap-yieldbox/blob/8de8ad15fe80c4e00843475d7d20fae4d0397003/contracts/interfaces/IYieldBox.sol#L56

3. https://github.com/sherlock-audit/2024-02-tapioca/blob/dc2464f420927409a67763de6ec60fe5c028ab0e/Tapioca-bar/contracts/Penrose.sol#L580

Tool used

Manual Review

Recommendation

(uint256 amountOut, )yieldBox.withdraw(_assetId, address(this), address(this), 0, feeShares);

uint256 rewardTokenId = twTap.rewardTokenIndex(_asset);

_distributeOnTwTap(amountOut, rewardTokenId, _asset, twTap);

Duplicate of #43

[sherlock-admin4]

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

takarez commented:

seem invalid to me

[maarcweiss]

This scenario: if feeAmount is greater than amountOut, calling _distributeOnTwTap will revert and block reward distribution. seems like will never be given because feeShares should be always equal or bigger than the actual amount withdrawn as if you check the withdraw function on the Yieldbox, it rounds down:

  } else {
            // amount may be lower than the value of share due to rounding, that's ok
            amount = share._toAmount(totalSupply[assetId], totalAmount, false);
        }

Let me know if I am incorrect whoever sumitted, thanks.

[nevillehuang]

request poc

[sherlock-admin3]

PoC requested from @ctf-sec

Requests remaining: 1

[ctf-sec]

This scenario: if feeAmount is greater than amountOut, calling _distributeOnTwTap will revert and block reward distribution. seems like will never be given because feeShares should be always equal or bigger than the actual amount withdrawn as if you check the withdraw function on the Yieldbox, it rounds down:

  } else {
            // amount may be lower than the value of share due to rounding, that's ok
            amount = share._toAmount(totalSupply[assetId], totalAmount, false);
        }

Let me know if I am incorrect whoever sumitted, thanks.

Yeah I agree, but I think the

 uint256 feeAmount = yieldBox.toAmount(_assetId, feeShares, false);

still not match the withdraw amount, which results in a port of reward not distributed.

[ctf-sec]

I think the worth case is that the feeShares equals to most of the total shares in the yield box:

 function testShares() public {

        uint256 share = 10 ether;
        uint256 total = 10 ether;
        uint256 amonut = 10 ether;

        amonut++;
        total+= 1e8;

        uint256 toAmount = share * amonut / total;

        console.log("toAmount: %d", toAmount);

        uint256 share2 = 10 ether;
        uint256 total2 = 0 ether;
        uint256 amonut2 = 0 ether;

        amonut2++;
        total2+= 1e8;

        uint256 toAmount2 = share2 * amonut2 / total2;

        console.log("toAmount2: %d", toAmount2);

        console.log((toAmount - toAmount2) / 1 ether); 

    }

In this case ,the majority of the reward is not distributed.

Anyway the root cause is not handling the return value of yieldBox.withdraw, which worth fixing.

[ctf-sec]

https://github.com/boringcrypto/YieldBox/blob/cd62ffc4b003e4de428b45169010e94b523caef4/contracts/YieldBoxRebase.sol#L51

totalAmount++;
totalShares_ += 1e8;

[CergyK]

I believe this is a duplicate of my issue #43 Have detailed an example where it can revert.

The reasoning is basically that both withdraw and toAmount should round down in most cases, but if withdraw rounds down and the division in toAmount has no remainder, toAmount returns a bigger value and the function reverts.

This situation is possible since toAmount is called after the withdraw, and thus is used on different values.

Maybe we can avoid coding the PoC for the same thing twice, and use @ctf-sec 's Poc for this

[cryptotechmaker]

Yes, it seems a duplicate of #43

[sherlock-admin4]

The protocol team fixed this issue in PR/commit https://github.com/Tapioca-DAO/Tapioca-bar/pull/353.