JuggerNaut63 - Unrestricted Token Exchange Functionality in MkrNgt Contract

[sherlock-admin4]

JuggerNaut63

High

Unrestricted Token Exchange Functionality in MkrNgt Contract

Summary

The MkrNgt allows unrestricted access to its token exchange functions mkrToNgt and ngtToMkr. This lack of access control permits any user to burn tokens from any address and mint tokens to any address, potentially leading to unauthorized token transfers and significant financial loss.

Vulnerability Detail

1. Malicious User Burns Tokens from Other Addresses:

   • Step 1: Malicious user (Attacker) knows that the mkrToNgt and ngtToMkr functions have no access control.
   • Step 2: Attacker gets permission (approval) to burn tokens from the victim's address. This can happen if the victim accidentally grants permission through an unsafe transaction.
   • Step 3: Attacker calls the mkrToNgt or ngtToMkr function with msg.sender as the victim's address and usr as the attacker's address.
   • Step 4: The function burns tokens from the victim's address and prints tokens to the attacker's address.

2. Sybil Attack:

   • Step 1: Attacker creates many fake accounts (Sybil accounts).
   • Step 2: Attacker uses these accounts to repeatedly call the mkrToNgt and ngtToMkr functions, trying different combinations to exploit the system.
   • Step 3: Without access control, attackers can try various ways to manipulate token exchanges and mint tokens to their own accounts.

Impact

• Unauthorized Token Transfers
• Financial Loss
• System Exploitation

Code Snippet

https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/ngt/src/MkrNgt.sol#L41-L53

Tool used

Manual Review

Recommendation

Implement strict access control mechanisms to restrict who can call the mkrToNgt and ngtToMkr functions. Consider using role-based access control or ownership checks to ensure only authorized users can perform token exchanges.

1. Using Ownable:

   
   +   import "@openzeppelin/contracts/access/Ownable.sol";

• contract MkrNgt {

• contract MkrNgt is Ownable {

• function mkrToNgt(address usr, uint256 mkrAmt) external {

• function mkrToNgt(address usr, uint256 mkrAmt) external onlyOwner { mkr.burn(msg.sender, mkrAmt); uint256 ngtAmt = mkrAmt * rate; ngt.mint(usr, ngtAmt); emit MkrToNgt(msg.sender, usr, mkrAmt, ngtAmt); }

• function ngtToMkr(address usr, uint256 ngtAmt) external {

• function ngtToMkr(address usr, uint256 ngtAmt) external onlyOwner { ngt.burn(msg.sender, ngtAmt); uint256 mkrAmt = ngtAmt / rate; // Rounding down, dust will be lost if it is not multiple of rate mkr.mint(usr, mkrAmt); emit NgtToMkr(msg.sender, usr, ngtAmt, mkrAmt); }

  
  2. Using Role-Based Access Control:
  ```solidity
  import "@openzeppelin/contracts/access/AccessControl.sol";

contract MkrNgt is AccessControl { bytes32 public constant EXCHANGER_ROLE = keccak256("EXCHANGER_ROLE");

constructor(address mkr_, address ngt_, uint256 rate_) {
    mkr = GemLike(mkr_);
    ngt = GemLike(ngt_);
    rate = rate_;
    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
    _setupRole(EXCHANGER_ROLE, msg.sender);
}

function mkrToNgt(address usr, uint256 mkrAmt) external onlyRole(EXCHANGER_ROLE) {
    mkr.burn(msg.sender, mkrAmt);
    uint256 ngtAmt = mkrAmt * rate;
    ngt.mint(usr, ngtAmt);
    emit MkrToNgt(msg.sender, usr, mkrAmt, ngtAmt);
}

function ngtToMkr(address usr, uint256 ngtAmt) external onlyRole(EXCHANGER_ROLE) {
    ngt.burn(msg.sender, ngtAmt);
    uint256 mkrAmt = ngtAmt / rate;
    mkr.mint(usr, mkrAmt);
    emit NgtToMkr(msg.sender, usr, ngtAmt, mkrAmt);
}

}

## PoC
```solidity
// SPDX-License-Identifier: AGPL-3.0-or-later
pragma solidity ^0.8.21;

import "forge-std/Test.sol";
import "../src/MkrNgt.sol";

contract MockGem is GemLike {
    mapping(address => uint256) public balances;
    mapping(address => mapping(address => uint256)) public allowances;

    function burn(address from, uint256 amount) external override {
        require(balances[from] >= amount, "Insufficient balance");
        balances[from] -= amount;
    }

    function mint(address to, uint256 amount) external override {
        balances[to] += amount;
    }

    function approve(address spender, uint256 amount) external {
        allowances[msg.sender][spender] = amount;
    }

    function balanceOf(address account) external view returns (uint256) {
        return balances[account];
    }
}

contract ExploitTest is Test {
    MkrNgt public mkrNgt;
    GemLike public mkr;
    GemLike public ngt;
    address public attacker;
    address public victim;

    function setUp() public {
        // Deploy mock tokens
        mkr = new MockGem();
        ngt = new MockGem();

        // Deploy the MkrNgt contract
        mkrNgt = new MkrNgt(address(mkr), address(ngt), 1);

        // Set up attacker and victim addresses
        attacker = address(0x1);
        victim = address(0x2);

        // Mint some tokens to the victim and attacker
        MockGem(address(mkr)).mint(victim, 1000);
        MockGem(address(ngt)).mint(victim, 1000);
        MockGem(address(mkr)).mint(attacker, 1000); // Mint tokens to the attacker

        // Approve the MkrNgt contract to burn victim's tokens
        vm.prank(victim);
        MockGem(address(mkr)).approve(address(mkrNgt), 1000);
        MockGem(address(ngt)).approve(address(mkrNgt), 1000);

        // Approve the MkrNgt contract to burn attacker's tokens
        vm.prank(attacker);
        MockGem(address(mkr)).approve(address(mkrNgt), 1000);
        MockGem(address(ngt)).approve(address(mkrNgt), 1000);
    }

    function testExploitWithoutAccessControl() public {
        // Step 1: Attacker calls mkrToNgt with victim's address
        vm.prank(attacker);
        mkrNgt.mkrToNgt(victim, 100);

        // Check balances after the exploit
        assertEq(MockGem(address(mkr)).balanceOf(victim), 900, "Victim's MKR balance should be reduced");
        assertEq(MockGem(address(ngt)).balanceOf(attacker), 100, "Attacker's NGT balance should be increased");

        // Step 2: Attacker calls ngtToMkr with victim's address
        vm.prank(attacker);
        mkrNgt.ngtToMkr(victim, 100);

        // Check balances after the exploit
        assertEq(MockGem(address(ngt)).balanceOf(victim), 900, "Victim's NGT balance should be reduced");
        assertEq(MockGem(address(mkr)).balanceOf(attacker), 1100, "Attacker's MKR balance should be increased");

        // If the exploit is successful, the test should pass
        emit log("Exploit successful, test passed.");
    }
}

forge test --match-path test/ExploitTest.sol [⠊] Compiling... [⠔] Compiling 1 files with Solc 0.8.21 [⠒] Solc 0.8.21 finished in 1.38s Compiler run successful!

Ran 1 test for test/ExploitTest.sol:ExploitTest [PASS] testExploitWithoutAccessControl() (gas: 78590) Logs: Exploit successful, test passed.

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.08ms (216.00µs CPU time)

Ran 1 test suite in 8.10ms (1.08ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

[telome]

Not an issue. mkrToNgt and ngtToMkr only burn from the msg.sender account.