Distribution Updates Can Be Gamed

[code423n4]

Handle

leastwood

Vulnerability details

Impact

Any account can deposit XDEFI into the XDEFIDistribution.sol contract for whatever reason. The difference in deposited XDEFI (received by locking tokens) and the underlying contract balance is utilised as a sort of yield distributed to protocol participants.

The updateDistribution() function is called whenever users want to update the _pointsPerUnit value, representing a user's claim on the underlying XDEFI tokens received externally. However, as there is no incentive or fixed interval by which updateDistribution() is called, users may unexpectedly lose value if they unlock tokens without first updating the _pointsPerUnit value. Similarly, a user could also frontrun an update to this value, effectively siphoning a small amount of yield. If the minimum lock period is sufficiently small, this can be repeated over and over again to generate additional yield as compared to other market participants. The user will then be able to utilise their capital by deploying it within other protocols when it is not feasible to siphon additional yield from XDEFIDistribution.

Proof of Concept

function updateDistribution() external {
    uint256 totalUnitsCached = totalUnits;

    require(totalUnitsCached > uint256(0), "NO_UNIT_SUPPLY");

    uint256 newXDEFI = _toUint256Safe(_updateXDEFIBalance());

    if (newXDEFI == uint256(0)) return;

    _pointsPerUnit += ((newXDEFI * _pointsMultiplier) / totalUnitsCached);

    emit DistributionUpdated(msg.sender, newXDEFI);
}

Tools Used

Manual code review

Recommended Mitigation Steps

Consider adding incentives for continuous and frequent calls to updateDistribution(). Chainlink keepers can be effectively used as a solution, or a similar implementation funded by XDEFI tokens.

Additionally, it may also be useful to replace the internal call _updateXDEFIBalance() with a call that updates the updateDistribution value. This will also help to maintain protocol liveness.

[deluca-mike]

While technically valid, this is not really an issue at all. The intended flow for a rewards distribution (in this case the XDEFI organization) is to, in one transaction, send XDEFI to the XDEFIDistribution contract, and call updateDistribution() in order to account for it. The reason for this flow is that the XDEFI organization treasury has the ability may have the ability to send XDEFI to an address, but not the ability to call updateDistribution(), and the XDEFI organization admin wallet may have the ability to call updateDistribution(), but not have the XDEFI needed for rewards. Thus, the XDEFI organization wallet would do a multi-call (something like a Gnosis Safe) which first instructs the treasury to send XDEFi to the XDEFIDistribution contract, and then calls updateDistribution().

The onus is on the reward distributor to call updateDistribution() in order to account for new rewards. The distributor doing something incorrect which results in the reward not being accounted for, and thus lost, is just as bad, and possible, as the distributor sending token to a different address. If we can't trust the distributor to use the contract properly, then we are already lost.

Now, a solution for other issues found in this audit just happened to be to precede all locks and unlocks with updateDistribution(), which is a bit similar to the Recommended Mitigation Steps here, expect done at the start, and not at the end.

[Ivshti]

closing

finding considered valid and low severity due to the explanation by the sponsor.