SplitPayment Manual Review Findings

SPT-01M: Slightly Inaccurate Documentation

Type	Severity	Location
Code Style		SplitPayment.sol:L68-L72

Description:

The documentation states that truncation due to incomplete share values will favour the sender by allowing dust to remain in their account. While partly true, this is not correct for transfers from self which will cause dust to be permanently locked in the contract and additionally in some cases not consuming the full allowance via transferFrom can cause contracts to misbehave, such as contracts using the deprecated safeApprove function of OpenZeppelin.

Impact:

Incorrect assumptions as to how allowance is consumed by the contract and how fund balances may be permanently non-zero within it can cause external contracts to misbehave.

Example:

Copy
68// FixedPointMath uses checked math so disallows overflow.
69// Integrity checks disallow `0` shares.
70// If there is any dust due to rounding error in the
71// calculations it will favour the sender by simply never
72// sending the rounding error to any recipient.
73uint256 amount_ = totalAmount_.fixedPointMul(shares_[i_].share);
74if (fromThis_) {
75    IERC20(token_).safeTransfer(shares_[i_].recipient, amount_);
76} else {
77    IERC20(token_).safeTransferFrom(
78        from_,
79        shares_[i_].recipient,
80        amount_
81    );
82}

Recommendation:

We advise either the documentation to be updated to highlight that funds can be locked in dust amounts within the contract or the code to be adjusted to account for dust properly by transmitting the remainder in the last iteration of the shareholder loop, either of which we consider an adequate alleviation to this exhibit.

Alleviation:

The documentation was updated to properly reflect that token amounts can indeed be locked in the contract in the self-sender scenario thus sufficiently alleviating this exhibit.