sherlock-audit / 2024-02-tapioca-judging

2 stars 2 forks source link

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

Open sherlock-admin3 opened 5 months ago

sherlock-admin3 commented 5 months ago

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());
        }
nevillehuang commented 5 months ago

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

dmitriia commented 5 months ago

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 commented 5 months ago

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