code-423n4 / 2024-02-tapioca-findings

1 stars 1 forks source link

_internalRemoteTransferSendPacket() can't send the difference back to the user #115

Open c4-bot-2 opened 7 months ago

c4-bot-2 commented 7 months ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/tapiocaOmnichainEngine/TapiocaOmnichainReceiver.sol#L261

Vulnerability details

Vulnerability details

in TapiocaOmnichainReceiver when the user executes MSG_REMOTE_TRANSFER If the srcChain amount request is bigger than the debited one, overwrite the amount to credit with the amount debited and send the difference back to the user.

lzCompose() -> _remoteTransferReceiver()->_internalRemoteTransferSendPacket()

    function _internalRemoteTransferSendPacket(
        address _srcChainSender,
        LZSendParam memory _lzSendParam,
        bytes memory _composeMsg
    ) internal returns (MessagingReceipt memory msgReceipt, OFTReceipt memory oftReceipt) {
...

        // If the srcChain amount request is bigger than the debited one, overwrite the amount to credit with the amount debited and send the difference back to the user.
        if (_lzSendParam.sendParam.amountLD > amountDebitedLD_) {
            // Overwrite the amount to credit with the amount debited
@>          _lzSendParam.sendParam.amountLD = amountDebitedLD_;
            _lzSendParam.sendParam.minAmountLD = amountDebitedLD_;
            // Send the difference back to the user
@>          _transfer(address(this), _srcChainSender, _lzSendParam.sendParam.amountLD - amountDebitedLD_);
        }

The above code, first modify _lzSendParam.sendParam.amountLD = amountDebitedLD_ Then call _transfer(address(this), _srcChainSender, _lzSendParam.sendParam.amountLD - amountDebitedLD_); This way the difference is always `0

Correctly transfer() the difference first, and then modify _lzSendParam.sendParam.amountLD

Impact

the difference token be left in the contract

Recommended Mitigation

    function _internalRemoteTransferSendPacket(
        address _srcChainSender,
        LZSendParam memory _lzSendParam,
        bytes memory _composeMsg
    ) internal returns (MessagingReceipt memory msgReceipt, OFTReceipt memory oftReceipt) {
...

        // If the srcChain amount request is bigger than the debited one, overwrite the amount to credit with the amount debited and send the difference back to the user.
        if (_lzSendParam.sendParam.amountLD > amountDebitedLD_) {
+          _transfer(address(this), _srcChainSender, _lzSendParam.sendParam.amountLD - amountDebitedLD_);
            // Overwrite the amount to credit with the amount debited
            _lzSendParam.sendParam.amountLD = amountDebitedLD_;
            _lzSendParam.sendParam.minAmountLD = amountDebitedLD_;
            // Send the difference back to the user
-           _transfer(address(this), _srcChainSender, _lzSendParam.sendParam.amountLD - amountDebitedLD_);
        }

Assessed type

Context

c4-sponsor commented 7 months ago

cryptotechmaker (sponsor) confirmed

c4-judge commented 7 months ago

dmvt marked the issue as primary issue

c4-judge commented 7 months ago

dmvt changed the severity to 2 (Med Risk)

c4-judge commented 7 months ago

dmvt marked the issue as selected for report

cryptotechmaker commented 7 months ago

https://github.com/Tapioca-DAO/tapioca-periph/pull/214/commits/c9170340568a736c1acdb8ad82a1ef71e66165f0