Cyfrin / 2023-08-sparkn

Other
10 stars 15 forks source link

DAI Tokens at Risk Due to Lack of address(0) Check in distribute #721

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

DAI Tokens at Risk Due to Lack of address(0) Check in distribute

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Distributor.sol#L147

Summary

The distribute function does not check if the winner is address(0). For some tokens Like USDC and USDT it does check internally if the sender and receiver are not address(0) and revert it (so it's not necessary for the function to check it), but the DAI token does not check for that and will not revert and send tokens to the 0 address.

Vulnerability Details

Since the dev described that the DAI token will be present in the contract the function _distribute should check if any of the winners are address(0).
Here is the DAI token code: https://etherscan.io/token/0x6b175474e89094c44da98b954eedeac495271d0f#code , the functions used to transfer the tokens internally does not check for the address(0) as seen here in the DAI contract:

function transfer(address dst, uint wad) external returns (bool) {
        return transferFrom(msg.sender, dst, wad);
    }
    function transferFrom(address src, address dst, uint wad)
        public returns (bool)
    {
        require(balanceOf[src] >= wad, "Dai/insufficient-balance");
        if (src != msg.sender && allowance[src][msg.sender] != uint(-1)) {
            require(allowance[src][msg.sender] >= wad, "Dai/insufficient-allowance");
            allowance[src][msg.sender] = sub(allowance[src][msg.sender], wad);
        }
        balanceOf[src] = sub(balanceOf[src], wad);
        balanceOf[dst] = add(balanceOf[dst], wad);
        emit Transfer(src, dst, wad);
        return true;
    }

Impact

DAI tokens could be permanently lost if sent to address(0) because lack of checking

Tools Used

Manual review, Etherscan

Recommendations

Add a built in check if any of the winners are 0 address because not all tokens do that check internally, specially DAI which the dev explicitly commented that it is going to be used.

PatrickAlphaC commented 1 year ago

marking as selected as the DAI mention brought up a great point.