code-423n4 / 2023-01-drips-findings

0 stars 2 forks source link

Upgraded Q -> 2 from #298 [1677237168746] #326

Closed c4-judge closed 1 year ago

c4-judge commented 1 year ago

Judge has assessed an item in Issue #298 as 2 risk. The relevant finding follows:

[01] MALICIOUS USER, WHO OWNS SPLITTABLE FUNDS, CAN CALL DripsHub.setSplits FUNCTION TO FRONTRUN OTHER USER'S DripsHub.split FUNCTION CALL, WHICH CAN BREAK AGREEMENT BETWEEN THESE USERS Based on the current implementation, the following DripsHub.split function is callable by anyone. This function's comment implies that the user, who owns the splittable funds, can call the DripsHub.setSplits function to frontrun the other user's DripsHub.split function call so the other user, who is one of the old splits receivers but not one of the new, cannot receive the corresponding funds. Here, the user, who owns the splittable funds, is assumed to be trusted. However, such user can become malicious in reality. For example, Alice, who owns the splittable funds, previously agrees to distribute some funds to Bob. Later, Alice becomes malicious and breaks the agreement without notifying Bob. After noticing that Bob has called the DripsHub.split function, Alice frontruns it by calling the DripsHub.setSplits function to remove Bob from the splits receivers. After the frontrunning, Bob's DripsHub.split function call does not distribute any funds to him so he loses the funds that he is entitled to under his and Alice's previous agreement.

As a mitigation, if the DripsHub.split function needs to remain callable by anyone, flashbots can be used to keep users' DripsHub.split transactions away from the mempool for counteracting frontrunning.

https://github.com/code-423n4/2023-01-drips/blob/main/src/DripsHub.sol#L336-L361

...
/// 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 user must be trusted with how funds sent to them will be splits,
/// in the end they can do with their funds whatever they want by changing the configuration.
...
function split(uint256 userId, IERC20 erc20, SplitsReceiver[] memory currReceivers)
    public
    whenNotPaused
    returns (uint128 collectableAmt, uint128 splitAmt)
{
    return Splits._split(userId, _assetId(erc20), currReceivers);
}
c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #201

c4-judge commented 1 year ago

GalloDaSballo marked the issue as satisfactory

c4-judge commented 1 year ago

GalloDaSballo marked the issue as unsatisfactory: Invalid