code-423n4 / 2022-09-artgobblers-findings

0 stars 0 forks source link

Incorrect Goo issuance #458

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/transmissions11/goo-issuance/blob/648e65e66e43ff5c19681427e300ece9c0df1437/src/LibGOO.sol#L27-L39

Vulnerability details

Impact

The rate of Goo issuance is defined by the documentation as $g'=\sqrt{mg}$ with $g(t)$ the Goo balance at time $t$, and $m$ the multiplier.

The solution of $g(t)$, given the initial balance $g_0$, is provided in the computeGooBalance function (link).

This solution however is wrong in the case $g_0 = 0$, since the correct solution would be $g(t) = 0$. It's clear that when the rate of issuance is $0$, the balance can't go up. The mistake rises when solving the differential equation by dividing both sides by $g$, which can't be done if it's zero.

The impact is that according to the documentation someone with 0 Goo wouldn't be able to mint. They will however mint extra Goo which may not be accounted by the protocol.

Proof of Concept

https://github.com/transmissions11/goo-issuance/blob/648e65e66e43ff5c19681427e300ece9c0df1437/src/LibGOO.sol#L27-L39

Recommended Mitigation Steps

The correct implementation would be adding the following line before L27.

if (lastBalanceWad == 0) return 0;

If you implement this however there would be no way to mint Goo in the first place. My recommendation is to just modify the documentation to account for this abnormal behaviour.

GalloDaSballo commented 1 year ago

Adding a 0 will cause a revert / underflow.

    function testgoo() public {
        require(gobblers.gooBalance(address(0)) == 0);
    }
Screenshot 2022-10-09 at 01 51 41

I think this is valid, but cannot imagine any specific scenario, where a Loss of value or a DOS can happen.

If the library was out of scope, i believe that the revert would still be a valid finding, specifically a Refactoring (as the revert is ungraceful vs a more UX friendly "Insufficient Goo Balance")

Because the library is in scope, in lack of a specific POC am downgrading to it Low

GalloDaSballo commented 1 year ago

L

hrkrshnn commented 1 year ago

This solution however is wrong in the case $g_0 = 0$ , since the correct solution would be $g(t) = 0$

There is no issue with the solution for the initial value $g(0) = 0$. The solution they are using is still correct. It's just that the differential equation has non-unique solutions for this initial value.


if (lastBalanceWad == 0) return 0;

I think adding this would mean that you would never be able to kick-start Goo production. An account will need to have initial Goo to accrue more Goo, but Goo starts with total supply of 0, so nobody would ever be able to kick-start the production.