Splits._setSplits function doesn't transfer splittable amount to receivers before changing

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L212-L220

Vulnerability details

Impact

Splits._setSplits function doesn't transfer splittable amount to receivers before changing.

Proof of Concept

When user wants to change split receivers he can call DripsHub.setSplit which will call Splits._setSplits function. https://github.com/code-423n4/2023-01-drips/blob/main/src/Splits.sol#L212-L220

    function _setSplits(uint256 userId, SplitsReceiver[] memory receivers) internal {
        SplitsState storage state = _splitsStorage().splitsStates[userId];
        bytes32 newSplitsHash = _hashSplits(receivers);
        emit SplitsSet(userId, newSplitsHash);
        if (newSplitsHash != state.splitsHash) {
            _assertSplitsValid(receivers, newSplitsHash);
            state.splitsHash = newSplitsHash;
        }
    }

The problem here is that in case if current list of split recipients is not empty and splittable amount of user for some ERC20 tokens is not 0, then it will not be splited before changing split list. This is done because user can have big amount of ERC20 tokens that he has splittable amount in and in it will be too expensive for him to call such tx that will find all tokens, where splittable balance is not 0 and split that amount.

However, this is not fair, as some receivers will not receive splits for previous amount of time when they are removed or their weight has decreased.

To split, anyone can call DripsHub.split for specific erc20 token they want to split amount. So once user calls Splits._setSplits, with new split list, anyone can front run it and call DripsHub.split in order to receive splittable amount.

I believe that this is unfair to receivers as they should receive amount that should be splitted for them, before split list has changed. I guess that some cooldown period can be added, when user wants to change split list, so old receivers can call split for the tokens they are interested in. And only after that cooldown period, split list will be changed.

Tools Used

VsCode

Recommended Mitigation Steps

Add cooldown period for changing split list.

[GalloDaSballo]

See #218 separate for now

[xmxanuel]

This was a design choice. It is technically very complex to implement this requirement.

Currently, the split config is global for a user. The same split config for each ERC20.

It would require tracking all ERC20 tokens that a receiver uses (in an array) and then applying the splitting for each token before the split config will be stored.

I think we mention this as a known trade-off in our documentation.

[CodeSandwich]

[dispute validity] What Manuel said.

[c4-sponsor]

CodeSandwich marked the issue as sponsor disputed

[GalloDaSballo]

I believe the concept of current settings changing past accruals to be a well-understood concept and I think the report shows an instance of it.

@xmxanuel can you please link to the documentation saying that _setSplits will break / alter past rewards?

[GalloDaSballo]

With the information that I have, I believe the finding to be valid, because it pertains to a retro-active change of settings, I believe the finding to be of Medium Severity

[c4-judge]

GalloDaSballo marked the issue as selected for report

[xmxanuel]

@GalloDaSballo It is a known trade-off that is not new. That is the reason why we implemented the ImmutableSplitsDriver.

[xmxanuel]

Obviously, as discussed the final judgment is done by you. :+1:

However, here we basically are aware of this problem already in the design and implement an alternative concept with the ImmutableSplitsDriver to address this issue to some extent.

If I understand your reasoning here correctly you are saying that the user might expect a different behaviour and therefore you see it as a security concern?

[GalloDaSballo]

Obviously, as discussed the final judgment is done by you. 👍

However, here we basically are aware of this problem already in the design and implement an alternative concept with the ImmutableSplitsDriver to address this issue to some extent.

If I understand your reasoning here correctly you are saying that the user might expect a different behaviour and therefore you see it as a security concern?

Thank you for the understanding @xmxanuel , ultimately the security concern is the possibility of retroactively changing accrued value, this is allowing a retro-active change.

I believe due to it we must flag it as a potential loss of yield.

Because of the trade-off and the understanding from your team, I think this can be a nofix as long as properly explained to end-users

[c4-judge]

GalloDaSballo marked the issue as primary issue

[0xA5DF]

I think this is pretty explicitly documented here:

    /// All split funds are split using the current splits configuration.
    /// Because the user can update their splits configuration at any time,
    /// it is possible that calling this function will be frontrun,
    /// and all the splittable funds will become splittable only using the new configuration.

The devs make it clear that in case a new split configuration is set it will be used for the entire amount received, they also add that the owner can even front run split() if they notice it on chain.

[GalloDaSballo]

Have consulted with another judge We both agree that because the comments explicitly mention the case, the finding is OOS as known

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope