search

status-im / ens-usernames

DApp to register usernames for Status Network
MIT License
19 stars 11 forks source link

UsernameRegistrar.slashUsername(bytes,uint256) performs a multiplication on the result of a division which may lead to loss of precision. #115

Closed ghost closed 4 years ago

ghost commented 4 years ago

UsernameRegistrar.slashUsername(bytes,uint256) performs a multiplication on the result of a division: -partialDeposit = amountToTransfer / 3 -amountToTransfer = partialDeposit * 2

Crytic: Check: divide-before-multiply | Impact: Medium | Confidence: Medium. Solidity integer division might truncate. As a result, performing a multiply after a division might lead to loss of precision.

Problem: If amountToTransfer(i.e. accounts[label].balance) is less than 3 (assuming this is possible at some point. If not, this can be ignored. ), then partialDeposit and hence amountToTransfer used in subsequent token transfer will be 0. The scenario where this may be relevant is when the balance happens to be 2. Currently, this will transfer 0 tokens to reserver and retain both tokens to the network controller. With the recommendation below, we would transfer 1 token (1/2) to reserver and retain 1 (1/2). This deviates from retaining 1/3 but may be considered fairer than retaining 100% perhaps.

Recommendation: Change this to amountToTransfer = (amountToTransfer * 2)/3, if this boundary condition may be triggered at any time and if it seems unfair.

3esmit commented 4 years ago

Fixed at 198574ec4226446be31ceda18579af793657bcd8