Skip to content
This repository has been archived by the owner on Sep 15, 2024. It is now read-only.

hyh - TOFTOptionsReceiverModule miss cross-chain transformation for deposit and lock amounts #87

Open
sherlock-admin3 opened this issue Mar 15, 2024 · 3 comments
Labels
High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin3
Copy link
Contributor

sherlock-admin3 commented Mar 15, 2024

hyh

high

TOFTOptionsReceiverModule miss cross-chain transformation for deposit and lock amounts

Summary

Cross-chain token decimals transformation is applied partially in TOFTOptionsReceiverModule's lockAndParticipateReceiver() and mintLendXChainSGLXChainLockAndParticipateReceiver().

Vulnerability Detail

Currently only first level amounts are being transformed in cross-chain TOFTOptionsReceiverModule, while the nested deposit and lock amounts involved aren't.

Whenever the decimals are different for underlying tokens across chains the absence of transformation will lead to magnitudes sized misrepresentation of user operations, which can result in core functionality unavailability (operations can constantly revert or become a noops due to running them with outsized or dust sized parameters) and loss of user funds (when an operation was successfully run, but with severely misrepresented parameters).

Impact

Probability can be estimated as medium due to prerequisite of having asset decimals difference between transacting chains, while the operation misrepresentation and possible fund loss impact described itself has high severity.

Likelihood: Medium + Impact: High = Severity: High.

Code Snippet

Only mintAmount is being transformed in mintLendXChainSGLXChainLockAndParticipateReceiver():

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/tOFT/modules/TOFTOptionsReceiverModule.sol#L72-L82

    function mintLendXChainSGLXChainLockAndParticipateReceiver(bytes memory _data) public payable {
        // Decode received message.
        CrossChainMintFromBBAndLendOnSGLData memory msg_ =
            TOFTMsgCodec.decodeMintLendXChainSGLXChainLockAndParticipateMsg(_data);

        _checkWhitelistStatus(msg_.bigBang);
        _checkWhitelistStatus(msg_.magnetar);

        if (msg_.mintData.mintAmount > 0) {
            msg_.mintData.mintAmount = _toLD(msg_.mintData.mintAmount.toUint64());
        }

But collateral deposit amount from CrossChainMintFromBBAndLendOnSGLData.mintData.collateralDepositData there isn't:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/gitmodule/tapioca-periph/contracts/interfaces/periph/IMagnetar.sol#L104-L111

struct CrossChainMintFromBBAndLendOnSGLData {
    address user;
    address bigBang;
    address magnetar;
    address marketHelper;
>>  IMintData mintData;
    LendOrLockSendParams lendSendParams;
}

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/gitmodule/tapioca-periph/contracts/interfaces/oft/IUsdo.sol#L136-L140

struct IMintData {
    bool mint;
    uint256 mintAmount;
>>  IDepositData collateralDepositData;
}

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/gitmodule/tapioca-periph/contracts/interfaces/common/ICommonData.sol#L22-L25

struct IDepositData {
    bool deposit;
>>  uint256 amount;
}

Similarly option lock's amount and fraction from LockAndParticipateData in lockAndParticipateReceiver():

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/tOFT/modules/TOFTOptionsReceiverModule.sol#L106-L121

    function lockAndParticipateReceiver(bytes memory _data) public payable {
        // Decode receive message
        LockAndParticipateData memory msg_ = TOFTMsgCodec.decodeLockAndParticipateMsg(_data);

        _checkWhitelistStatus(msg_.magnetar);
        _checkWhitelistStatus(msg_.singularity);
        if (msg_.lockData.lock) {
            _checkWhitelistStatus(msg_.lockData.target);
        }
        if (msg_.participateData.participate) {
            _checkWhitelistStatus(msg_.participateData.target);
        }

        if (msg_.fraction > 0) {
            msg_.fraction = _toLD(msg_.fraction.toUint64());
        }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/gitmodule/tapioca-periph/contracts/interfaces/periph/IMagnetar.sol#L135-L142

struct LockAndParticipateData {
    address user;
    address singularity;
    address magnetar;
    uint256 fraction;
>>  IOptionsLockData lockData;
    IOptionsParticipateData participateData;
}

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/gitmodule/tapioca-periph/contracts/interfaces/tap-token/ITapiocaOptionLiquidityProvision.sol#L30-L36

struct IOptionsLockData {
    bool lock;
    address target;
    uint128 lockDuration;
>>  uint128 amount;
>>  uint256 fraction;
}

Tool used

Manual Review

Recommendation

Consider adding these local decimals transformations, e.g.:

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/tOFT/modules/TOFTOptionsReceiverModule.sol#L80-L82

        if (msg_.mintData.mintAmount > 0) {
            msg_.mintData.mintAmount = _toLD(msg_.mintData.mintAmount.toUint64());
        }
+       if (msg_.mintData.collateralDepositData.amount > 0) {
+           msg_.mintData.collateralDepositData.amount = _toLD(msg_.mintData.collateralDepositData.amount.toUint64());
+       }

https://github.com/sherlock-audit/2024-02-tapioca/blob/main/TapiocaZ/contracts/tOFT/modules/TOFTOptionsReceiverModule.sol#L112-L114

        if (msg_.lockData.lock) {
            _checkWhitelistStatus(msg_.lockData.target);
+           if (msg_.lockData.amount > 0) msg_.lockData.amount = _toLD(msg_.lockData.amount.toUint64());
+           if (msg_.lockData.fraction > 0) msg_.lockData.fraction = _toLD(msg_.lockData.fraction.toUint64());
        }
@sherlock-admin2 sherlock-admin2 added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Mar 16, 2024
@github-actions github-actions bot added the High A valid High severity issue label Mar 20, 2024
@nevillehuang
Copy link
Collaborator

@dmitriia Could you please provide a valid explicit example for supported chains (Arbitrum, Mainnet, Optimism, Avalanche) to validate your issue?

@dmitriia
Copy link
Collaborator

For example, the list of gas token LSDs that can be used as a collateral in BB isn't final. msg_.mintData.collateralDepositData.amount, which conversion is missed, can be the amount of LSD to be put in as a collateral for USDO minting.

That is, if after deployment a LSD be accepted that have different decimals across supported chains, this will have an impact of magnitudes.

@sherlock-admin4
Copy link
Contributor

The protocol team fixed this issue in PR/commit Tapioca-DAO/TapiocaZ#178.

@sherlock-admin3 sherlock-admin3 changed the title Zealous Pineapple Duck - TOFTOptionsReceiverModule miss cross-chain transformation for deposit and lock amounts hyh - TOFTOptionsReceiverModule miss cross-chain transformation for deposit and lock amounts Mar 31, 2024
@sherlock-admin3 sherlock-admin3 added the Reward A payout will be made for this issue label Mar 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
High A valid High severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

5 participants