code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-07-golom/blob/main/contracts/core/GolomTrader.sol#L381 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L744 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L942 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L994 https://github.com/code-423n4/2022-07-golom/blob/main/contracts/vote-escrow/VoteEscrowCore.sol#L1166

Vulnerability details

Impact

Severity: Medium

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.

Tools Used

Truffle,hardhat, remix

Recommended Mitigation Steps

Consider ordering multiplication before division.

Proof of Concept

If n is greater than oldSupply, coins will be zero. For example, with oldSupply = 5; n = 10, interest = 2, coins will be zero. If (oldSupply * interest / n) was used, coins would have been 1. In general, it's usually a good idea to re-arrange arithmetic to perform multiplication before division, unless the limit of a smaller type makes this dangerous.

Exploit Scenario:


contract A {
    function f(uint n) public {
        coins = (oldSupply / n) * interest; } }```

zeroexdead commented 2 years ago

Duplicate of #833

dmvt commented 2 years ago

There is a loss of precision, but its impact is minimal. Downgrading to QA

code-423n4 / 2022-07-golom-findings