User would lose funds when rate is zero

Summary

The missing zero value check in the constructor of MkrNgt contract can lead to a situation where rate state variable is set to zero during deployment. This can then lead to users losing their mkr funds when calling MkrNgt.mkrToNgt() where the mkr token is burned but the user does get any ngt minted to their account.

Root Cause

In MkrNgt.sol:38 there is a missing zero value check in the constructor to ensure that rate_ parameter is not zero.

When rate_ is zero, any user with mkr token calling the `MkrNgt.mkrToNgt() will be minted zero ngt tokens while getting their mkr burned since uint256 ngtAmt == 0 (MkrNgt.sol:43) - https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/ngt/src/MkrNgt.sol#L42-L44

Internal pre-conditions

rate_ parameter is zero during deployment of MkrNgt contract.

External pre-conditions

No response

Attack Path

No response

Impact

Loss of User funds
Contract redeployment

PoC


// SPDX-License-Identifier: AGPL-3.0-or-later

pragma solidity ^0.8.21;

import "dss-test/DssTest.sol";

import { Ngt } from "src/Ngt.sol";
import { MkrNgt } from "src/MkrNgt.sol";

contract Mkr is Ngt {}

contract MkrNgtLossTest is DssTest {
    Mkr     mkr;
    Ngt     ngt;
    MkrNgt  mkrNgt;

    event MkrToNgt(address indexed caller, address indexed usr, uint256 mkrAmt, uint256 ngtAmt);
    event NgtToMkr(address indexed caller, address indexed usr, uint256 ngtAmt, uint256 mkrAmt);

    function setUp() public {
        mkr = new Mkr();
        ngt = new Ngt();
        mkrNgt = new MkrNgt(address(mkr), address(ngt), 0);
        mkr.mint(address(this), 1_000_000 * WAD);
        mkr.rely(address(mkrNgt));
        mkr.deny(address(this));
        ngt.rely(address(mkrNgt));
        ngt.deny(address(this));
    }

    function testUserMkrLoss() public {
        assertEq(mkr.balanceOf(address(this)), 1_000_000 * WAD);
        assertEq(mkr.totalSupply(),            1_000_000 * WAD);
        assertEq(ngt.balanceOf(address(this)), 0);
        assertEq(ngt.totalSupply(),            0);

        mkr.approve(address(mkrNgt), 400_000 * WAD);
        vm.expectEmit(true, true, true, true);
        emit MkrToNgt(address(this), address(this), 400_000 * WAD,  400_000 * WAD * 0);
        mkrNgt.mkrToNgt(address(this), 400_000 * WAD);
        assertEq(mkr.balanceOf(address(this)), 600_000 * WAD);
        assertEq(mkr.totalSupply(),            600_000 * WAD);
        assertEq(ngt.balanceOf(address(this)), 0);
        assertEq(ngt.totalSupply(),            0);
    }

}

Mitigation

Add a require check to ensure that rate_ parameter is non-zero value.

sherlock-audit / 2024-06-makerdao-endgame-judging

cryptphi - User would lose funds when rate is zero #73