Missing scaling factor in recordKeyPurchase?

[code423n4]

Handle

cmichel

Vulnerability details

The Unlock.recordKeyPurchase function computes the maxTokens as:

maxTokens = IMintableERC20(udt).balanceOf(address(this)) * valueInETH / (2 + 2 * valueInETH / grossNetworkProduct) / grossNetworkProduct;

Note that grossNetworkProduct was already increased by valueInETH in the code before.
Meaning, the (2 + 2 * valueInETH / grossNetworkProduct) part of the computation will almost always be 2 as usually grossNetworkProduct > 2 * valueInETH, and thus the 2 * valueInETH / grossNetworkProduct is zero by integer division.

Impact

The maxTokens curve might not be computed as intended and lead to being able to receive more token rewards than intended.

Recommended Mitigation Steps

The comment "we distribute tokens using asymptotic curve between 0 and 0.5" should be more clear to indicate how exactly the curve looks like.
It could be that a floating-point number was desired instead of the integer division in 2 * valueInETH / grossNetworkProduct. In that case, consider adding a scaling factor to this term and divide by it at the end of the computation again.

[julien51]

I am not fully sure I understand what the problem is here?

[0xleastwood]

I think the warden is raising an issue where 2 * valueInEth / grossNetworkProduct will more than likely truncate and return 0. I think this is a valid finding.

[julien51]

Hum, we did some tests and could not reproduce here.

[0xleastwood]

I'm not sure how 2 * valueInETH / grossNetworkProduct does not always lead to some truncation. grossNetworkProduct is equal to valueInETH in the first call but always greater than valueInETH in any subsequent calls.