Tapioca DAO - 0xTheC0der's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L236-L238

Vulnerability details

Impact

When trying to wrap a native token into a TapiocaOFT by calling the suggested helper method MagnetarV2.burst(...) with _action.id == TOFT_WRAP and _action.value > 0, the user has to supply twice the msg.value since _action.value is counted twice in valAccumulator see L215 and L237 while only wrapping the provided _action.value once, see L240.
In the end, the double counted action value in valAccumulator is required to be equal to msg.value, see L714.

As a consequence, the user cannot use MagnetarV2.burst(...) as intened to wrap a native token into a TapiocaOFT or ends up paying the double amount for it, i.e. loss of funds.

Proof of Concept

Due to another bug (TapiocaOFT.wrapNative(...) is not implemented, called in L239) which is discussed in a separate report, creating a PoC test case is not possible without modifying the original contracts.
Therefore, this PoC relies on a commented version of the MagnetarV2.burst(...) method which makes this non-complex issue clearly visible:

function burst(
    Call[] calldata calls
) external payable returns (Result[] memory returnData) {
    uint256 valAccumulator;

    uint256 length = calls.length;
    returnData = new Result[](length);

    for (uint256 i = 0; i < length; i++) {
        Call calldata _action = calls[i];
        if (!_action.allowFailure) {
            require(
                _action.call.length > 0,
                string.concat(
                    "MagnetarV2: Missing call for action with index",
                    string(abi.encode(i))
                )
            );
        }

        unchecked { // @audit-info 1st time counting action value
            valAccumulator += _action.value;
        }

        if (_action.id == PERMIT_ALL) {
            _permit(
                _action.target,
                _action.call,
                true,
                _action.allowFailure
            );
        } else if (_action.id == PERMIT) {
            _permit(
                _action.target,
                _action.call,
                false,
                _action.allowFailure
            );
        } else if (_action.id == TOFT_WRAP) {
            WrapData memory data = abi.decode(_action.call[4:], (WrapData));
            _checkSender(data.from);
            if (_action.value > 0) {
                unchecked { // @audit-issue 2nd time counting action value
                    valAccumulator += _action.value;
                }
                ITapiocaOFT(_action.target).wrapNative{ // @audit method not yet implemented in TapiocaOFT
                    value: _action.value // @audit-info action value only wrapped once
                }(data.to);
            } else {
                ITapiocaOFT(_action.target).wrap(
                    msg.sender,
                    data.to,
                    data.amount
                );
            }
        // ...
        // @audit-info skipped other else-if action branches for readability
        // ...
        } else {
            revert("MagnetarV2: action not valid");
        }
    }

    require(msg.value == valAccumulator, "MagnetarV2: value mismatch"); // @audit-info checking accumulated value
}

Tools Used

VS Code, Hardhat

Recommended Mitigation Steps

Remove the superfluous second instance of counting the action value. Apply the diff below in tapioca-periph-audit:

diff --git a/contracts/Magnetar/MagnetarV2.sol b/contracts/Magnetar/MagnetarV2.sol
index 6bbaaf5..2884fea 100644
--- a/contracts/Magnetar/MagnetarV2.sol
+++ b/contracts/Magnetar/MagnetarV2.sol
@@ -233,9 +233,6 @@ contract MagnetarV2 is Ownable, MagnetarV2Storage {
                 WrapData memory data = abi.decode(_action.call[4:], (WrapData));
                 _checkSender(data.from);
                 if (_action.value > 0) {
-                    unchecked {
-                        valAccumulator += _action.value;
-                    }
                     ITapiocaOFT(_action.target).wrapNative{
                         value: _action.value
                     }(data.to);

Assessed type

Payable