0

I have

    function setNonRecurringFlow(uint256 _child, int256 _remainingSwapCredit) external;

    function setRecurringFlow(
        uint256 _child,
        int256 _maxSwapCredit,
        int256 _remainingSwapCredit,
        int _swapCreditPeriod, int _timeEnteredSwapCredit) external;

interface for my (ERC-1155, but that does not matter) contract.

setNonRecurringFlow is similar to approval() in ERC-20 as it allows to transfer _remainingSwapCredit tokens.

setRecurringFlow allows to transfer up to _maxSwapCredit per _swapCreditPeriod seconds (with the swap credit for the first period set to _remainingSwapCredit).

It has the same exploit as ERC-20 approval().

What is the best fix for this exploit of my contract?

The first thing that comes to mind is to check like as in the second variant of that question that the old settings (including whether the last call was setNonRecurringFlow or setRecurringFlow). But that requires too many function parameters, what is not good, even if allowed by Solidity compiler (or is so many parameters not allowed?)

I don't know what is the solution analogous to the first variant of that question.

Any ideas?

Here is the most important extracts from the code:

    struct TokenFlow {
        uint256 parentToken;
        int256 maxSwapCredit;
        int swapCreditPeriod;
        int timeEnteredSwapCredit; // zero means not in a swap credit
        int lastSwapTime; // ignored when not in a swap credit
        int256 remainingSwapCredit;
        bool enabled;
        bool recurring;
    }

    // User can set negative values. It is a nonsense but does not harm.
    function setRecurringFlow(
        uint256 _child,
        int256 _maxSwapCredit,
        int256 _remainingSwapCredit,
        int _swapCreditPeriod, int _timeEnteredSwapCredit) external
    {
        TokenFlow storage _flow = tokenFlow[_child];

        require(msg.sender == tokenOwners[_flow.parentToken]);
        // require(_remainingSwapCredit <= _maxSwapCredit); // It is caller's responsibility.

        _flow.maxSwapCredit = _maxSwapCredit;
        _flow.swapCreditPeriod = _swapCreditPeriod;
        _flow.timeEnteredSwapCredit = _timeEnteredSwapCredit;
        _flow.remainingSwapCredit = _remainingSwapCredit;
        _flow.recurring = true;
    }

    // User can set negative values. It is a nonsense but does not harm.
    function setNonRecurringFlow(uint256 _child, int256 _remainingSwapCredit) external {
        TokenFlow storage _flow = tokenFlow[_child];

        require(msg.sender == tokenOwners[_flow.parentToken]);
        // require(_remainingSwapCredit <= _maxSwapCredit); // It is caller's responsibility.

        _flow.remainingSwapCredit = _remainingSwapCredit;
        _flow.recurring = false;
    }

    function exchangeToParent(uint256 _id, uint256 _amount, uint _levels, bytes calldata _data) external {
        // Intentionally no check for `msg.sender`.
        if (_levels == 0) {
            return;
        }
        uint256 _currentId = _id;
        TokenFlow storage _flow;
        for (uint i = 0; i < _levels; ++i) {
            _flow = tokenFlow[_currentId];
            int _currentTimeResult = _currentTime();
            uint256 _maxAllowedFlow;
            bool _inSwapCreditResult;
            if (_flow.recurring) {
                _inSwapCreditResult = _inSwapCredit(_flow, _currentTimeResult);
                _maxAllowedFlow = _maxRecurringSwapAmount(_flow, _currentTimeResult, _inSwapCreditResult);
            } else {
                _maxAllowedFlow = _flow.remainingSwapCredit < 0 ? 0 : uint256(_flow.remainingSwapCredit);
            }
            require(_amount <= _maxAllowedFlow);
            uint256 _balance = balances[_currentId][msg.sender];
            require(_amount <= _balance);
            if (_flow.recurring && !_inSwapCreditResult) {
                _flow.timeEnteredSwapCredit = _currentTimeResult;
                _flow.remainingSwapCredit = _flow.maxSwapCredit;
            }
            _flow.lastSwapTime = _currentTimeResult; // TODO: no strictly necessary if !_flow.recurring
            require(_amount < 1<<128);
            _flow.remainingSwapCredit -= int256(_amount);
            _currentId = tokenFlow[_currentId].parentToken;
        }
        // if (_id == _flow.parentToken) return; // not necessary
        _doBurn(msg.sender, _id, _amount);
        _doMint(msg.sender, _flow.parentToken, _amount, _data);
    }
  • You're asking us to suggest a fix for the exploit yielded by these two functions, but how exactly are we supposed to do so without seeing their code??? – goodvibration Oct 13 at 14:20
  • No. Please post within your question, in plain text (no links or images) - all the relevant code, and only the relevant code. – goodvibration Oct 13 at 14:29
  • @goodvibration Done. – porton Oct 13 at 14:34
0

In each one of these functions, you can add the preliminary requirement:

require(_remainingSwapCredit == 0 || _flow.remainingSwapCredit == 0, "RSC must be reset");

Then post publicly, that whenever users "approve", they must ensure that their RSC is reset.

This will require them to execute two transactions whenever their RSC is not reset.

Implementation note: you may want to implement the requirement above as a modifier.

| improve this answer | |
  • I thought of this solution. This solution has a deficiency: It interrupts the state of allowed "swaps" (transfers to another token) with a time period when transfers become disallowed. That's at least a significant inconvenience for users. Is there a better solution? – porton Oct 13 at 14:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.