Rebalancing mTapiocaOFT of native token forces admin to pay for rebalance amount

[code423n4]

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L197-L208 https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/tOFT/mTapiocaOFT.sol#L141-L146

Vulnerability details

Impact

The mTapiocaOFT.sol contract is a special TOFT implementation that can balance its supply. The supply of the token is balanced across chains by using Stargate. When balancing a mTapiocaOFT the caller needs to provide some native token to pay the Stargate fees.

The issue with the current implementation is that the fee check for mTapiocaOFTs that represent native tokens is incorrect, forcing the admin to pay for the rebalance amount plus the fee.

Proof of Concept

A mTapiocaOFT can be rebalanced if the owner of an approved balancer contract calls rebalance in Balancer.sol. As mentioned above, the rebalance operation uses Stargate to send tokens across chain. In order to pay the Stargate fees, some ETH (i.e. a native token) needs to be passes with any calls to Stargate and thus the rebalance method needs to be payable (which it is). The issues comes where the value passed to the call is validated:

        //extract
        ITapiocaOFT(_srcOft).extractUnderlying(_amount);

        //send
        bool _isNative = ITapiocaOFT(_srcOft).erc20() == address(0);
        if (_isNative) {
            if (msg.value <= _amount) revert FeeAmountNotSet();
            _sendNative(_srcOft, _amount, _dstChainId, _slippage);
        } else {
            if (msg.value == 0) revert FeeAmountNotSet();
            _sendToken(_srcOft, _amount, _dstChainId, _slippage, _ercData);
        }

As you can see, for native tokens the check is (msg.value <= _amount). However, we're already extracting the native token when calling extractUnderlying:

    function extractUnderlying(uint256 _amount) external {
        require(balancers[msg.sender], "TOFT_auth");

        bool _isNative = erc20 == address(0);
        if (_isNative) {
            _safeTransferETH(msg.sender, _amount);
        } else {
            IERC20(erc20).safeTransfer(msg.sender, _amount);
        }

        emit Rebalancing(msg.sender, _amount, _isNative);
    }

So in order to rebalance amount x across chain, the Balancer.sol owner would have to call rebalance with msg.value = x + fee, and the actual amount being rebalanced would be 2 * x.

However, as I've highlighted in another issue, the underlying call will luckily fail, but this is a distinct issue that should also be resolved.

Tools Used

Manual review

Recommended Mitigation Steps

The fee check should be identical for both native and non native mTapiocaOFT representations:

diff --git a/contracts/Balancer.sol b/contracts/Balancer.sol
index b58058d..aa09a07 100644
--- a/contracts/Balancer.sol
+++ b/contracts/Balancer.sol
@@ -182,6 +182,7 @@ contract Balancer is Owned {
         onlyValidDestination(_srcOft, _dstChainId)
         onlyValidSlippage(_slippage)
     {
+        if (msg.value == 0) revert FeeAmountNotSet();
         if (connectedOFTs[_srcOft][_dstChainId].rebalanceable < _amount)
             revert RebalanceAmountNotSet();

@@ -200,10 +201,8 @@ contract Balancer is Owned {
         //send
         bool _isNative = ITapiocaOFT(_srcOft).erc20() == address(0);
         if (_isNative) {
-            if (msg.value <= _amount) revert FeeAmountNotSet();
             _sendNative(_srcOft, _amount, _dstChainId, _slippage);
         } else {
-            if (msg.value == 0) revert FeeAmountNotSet();
             _sendToken(_srcOft, _amount, _dstChainId, _slippage, _ercData);
         }

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as duplicate of #179

[c4-pre-sort]

minhquanym marked the issue as duplicate of #813

[c4-pre-sort]

minhquanym marked the issue as not a duplicate

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-sponsor]

0xRektora marked the issue as disagree with severity

[0xRektora]

Should be medium. Inconvenient for sure but user funds are not at risk.

[c4-sponsor]

0xRektora marked the issue as sponsor confirmed

[c4-judge]

dmvt changed the severity to 2 (Med Risk)

[c4-judge]

dmvt marked the issue as selected for report