Incorrect way of checking for overflow opened totalsupply to overflow breaking the invariant

Summary

The mint function in the provided Solidity code lacks proper overflow checks, which can lead to critical issues in the smart contract. It was noted that the was check for overflow, yes there was but for balanceOf[to] = balanceOf[to] + value; but not for totalSupply = totalSupply + value; https://github.com/sherlock-audit/2024-06-makerdao-endgame/blob/main/endgame-toolkit/src/SDAO.sol#L278

Vulnerability Detail

The totalSupply is incremented directly without any overflow protection. If the value is sufficiently large, the addition can cause an overflow, resulting in an incorrect and misleading totalSupply

 function mint(address to, uint256 value) external auth {
        require(to != address(0) && to != address(this), "SDAO/invalid-address");
        unchecked {
            // Note: safe as the sum of all balances is equal to `totalSupply`;
            // there is already an overvlow check below
            balanceOf[to] = balanceOf[to] + value;
        }
        totalSupply = totalSupply + value;

        emit Transfer(address(0), to, value);
    }

The assumption is that at any given time, the sum of the balances of all accounts should exactly match the totalSupply. The mint function is responsible for creating new tokens and adding them to a specific account (to). When minting new tokens, the totalSupply is increased by the amount minted. The comment suggests that since the function directly increases both the balanceOf[to] and the totalSupply by the same amount, and assuming there are no other bugs or vulnerabilities, the sum of all balances should still match the totalSupply after the operation.

Let's break down a simple example to understand the significance of this invariant:

totalSupply = 1000 balanceOf[Alice] = 500 balanceOf[Bob] = 500

The sum of all balances (balanceOf[Alice] + balanceOf[Bob]) is 1000, which matches totalSupply as said in the inline comment:

            // Note: safe as the sum of all balances is equal to `totalSupply`;
            // there is already an overvlow check below

The contract mints 100 new tokens to Charlie. totalSupply is increased by 100 balanceOf[Charlie] is increased by 100

Post mint

totalSupply = 1100 balanceOf[Alice] = 500 balanceOf[Bob] = 500 balanceOf[Charlie] = 100

The sum of all balances (balanceOf[Alice] + balanceOf[Bob] + balanceOf[Charlie]) is now 1100, which matches the new totalSupply.

But all that happened will not happen because it's an assumption. Total supply was never checked for overflow, which we can see in the mint function and is highly likely to happen.

Now

totalSupply = 2^256 - 10 (very close to the maximum uint256 value)

balanceOf[Alice] = 500 balanceOf[Bob] = 500 balanceOf[Charlie] = 0 The sum of all balances is balanceOf[Alice] + balanceOf[Bob] + balanceOf[Charlie] = 500 + 500 + 0 = 1000, and totalSupply is 2^256 - 10.

An auth calls the mint function to mint 20 new tokens to Charlie totalSupply becomes 2^256 - 10 + 20, which causes an overflow. Since 2^256 is the maximum value for a uint256, adding any number greater than 2^256 - totalSupply causes the value to wrap around.

totalSupply = 10 (due to overflow) balanceOf[Alice] = 500 balanceOf[Bob] = 500 balanceOf[Charlie] = 20

the sum of all balances is balanceOf[Alice] + balanceOf[Bob] + balanceOf[Charlie] = 500 + 500 + 20 = 1020, but totalSupply is 10. This breaks the invariant that the sum of all balances should equal totalSupply.

Impact

Break the assumption that it can't overflow.

Code Snippet

 function mint(address to, uint256 value) external auth {
        require(to != address(0) && to != address(this), "SDAO/invalid-address");
        unchecked {
            // Note: safe as the sum of all balances is equal to `totalSupply`;
            // there is already an overvlow check below
            balanceOf[to] = balanceOf[to] + value;
        }
        totalSupply = totalSupply + value;

        emit Transfer(address(0), to, value);
    }

Tool used

Manual Review

Recommendation

function mint(address to, uint256 value) external auth {
    require(to != address(0) && to != address(this), "SDAO/invalid-address");

    // SafeMath-like checks for overflow
    uint256 newBalance = balanceOf[to] + value;
    require(newBalance >= balanceOf[to], "SDAO/balance-overflow");
    balanceOf[to] = newBalance;

    uint256 newTotalSupply = totalSupply + value;
    require(newTotalSupply >= totalSupply, "SDAO/totalSupply-overflow");
    totalSupply = newTotalSupply;

    emit Transfer(address(0), to, value);
}

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

EFCCWEB3 - Incorrect way of checking for overflow opened totalsupply to overflow breaking the invariant #11