c4-bot-6 commented 4 months ago

Lines of code

https://github.com/Tapioca-DAO/tap-token/blob/20a83b1d2d5577653610a6c3879dff9df4968345/contracts/options/TapiocaOptionBroker.sol#L74 https://github.com/Tapioca-DAO/tap-token/blob/20a83b1d2d5577653610a6c3879dff9df4968345/contracts/options/TapiocaOptionBroker.sol#L266 https://github.com/Tapioca-DAO/tap-token/blob/20a83b1d2d5577653610a6c3879dff9df4968345/contracts/options/TapiocaOptionBroker.sol#L284

Vulnerability details

Impact

Lack of consideration for underlying YieldBox token decimals when computing computeMinWeight

Proof of Concept

TapiocaOptionBroker has this state virtual total amount

    /// @dev Virtual total amount to add to the total when computing twAML participation right. Default 10_000 * 1e18.
    uint256 private VIRTUAL_TOTAL_AMOUNT = 10_000 ether;

when computing the min voting power, the function is called

bool hasVotingPower =
    lock.ybShares >= computeMinWeight(pool.totalDeposited + VIRTUAL_TOTAL_AMOUNT, MIN_WEIGHT_FACTOR);

we are adding

pool.totalDeposited + VIRTUAL_TOTAL_AMOUNT

what is pool.totalDeposited?

pool.totalDeposited is the locked yield box shares

// Save new weight
pool.totalDeposited += lock.ybShares;

when user calling lock via TapiocaOptionLiquidityProvision contract,

the ybShares is the amount of share user transferred:

{
    bool isErr =
        pearlmit.transferFromERC1155(msg.sender, address(this), address(yieldBox), sglAssetID, _ybShares);
    if (isErr) {
        revert TransferFailed();
    }
}
activeSingularities[_singularity].totalDeposited += _ybShares;

// Create the lock position
tokenId = ++tokenCounter;
LockPosition storage lockPosition = lockPositions[tokenId];
lockPosition.lockTime = uint128(block.timestamp);
lockPosition.sglAssetID = uint128(sglAssetID);
lockPosition.lockDuration = _lockDuration;
lockPosition.ybShares = _ybShares;

but underlying ybShares decimals is the token decimals in yield box contracts

   function decimals(uint256 assetId) external view returns (uint8) {
        return
            uriBuilder.decimals(
                assets[assetId],
                nativeTokens[assetId].decimals
            );
    }

which is calling the function

function decimals(
    Asset calldata asset,
    uint8 nativeDecimals
) external view returns (uint8) {
    if (asset.tokenType == TokenType.ERC1155) {
        return 0;
    } else if (asset.tokenType == TokenType.ERC20) {
        IERC20 token = IERC20(asset.contractAddress);
        return token.safeDecimals();
    } else {
        return nativeDecimals;
    }
}

the decimal is the same as underlying token decimals.

this means if the underlying decimal is USDC / USDT, the decimal for yield box will be 6 instead of 18

then we are trying to add a 6 decimals number with 18 decimals default virtual amount setting

// virtual amount is not the same decimals!
bool hasVotingPower =
    lock.ybShares >= computeMinWeight(pool.totalDeposited + VIRTUAL_TOTAL_AMOUNT, MIN_WEIGHT_FACTOR);

which makes the state pool.totalDeposited that derives from yield box shares too small and void out the position that has hasVotingPower for yield box that has small underlying token decimal

Note in the current implementation in TapiocaOptionBroker.sol

the problem can be resolved by resetting VIRTUAL_TOTAL_AMOUNT

  function setVirtualTotalAmount(uint256 _virtualTotalAmount) external onlyOwner {
        VIRTUAL_TOTAL_AMOUNT = _virtualTotalAmount;
    }

but if we search across the codebase, the function above is never called even in test file, which shows that the protocol has no awareness for this issue before.

So the safe way to resolve this is avoid default unsafe VIRTUAL_TOTAL_AMOUNT amount

Tools Used

Manual Review

Recommended Mitigation Steps

Scale the VIRTUAL_TOTAL_AMOUNT = 10000 * 10 ** yieldBox.decimals()

Assessed type

Decimal

c4-sponsor commented 3 months ago

0xRektora (sponsor) acknowledged

c4-sponsor commented 3 months ago

0xRektora marked the issue as disagree with severity

0xRektora commented 3 months ago

I'll put it as informational. Issue is valid but not in the context of Tapioca. Users can lock with USDO only, which means only USDO YB asset will be in TOLP, and USDO is a 1e18 decimal asset.

c4-judge commented 3 months ago

dmvt marked the issue as unsatisfactory: Overinflated severity