Fee on Transfer tokens cause incorrect accounting

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L296-L322

Vulnerability details

Impact

Incorrect accounting for fee on transfer tokens

Proof of Concept

Delegate accepts all ERC20 token types as specified by the contest page. Some ERC20 tokens include a fee on transfer which means means that the amount of tokens sent to the contract will be different from the tokens received.

This is an issue in the create function which takes in the ERC20 tokens and then increments balances assuming this corresponds with the tokens transferred. However this does not hold true for fee-on-transfer tokens:

Within create:

StorageHelpers.incrementBalance(balances, delegateInfo.delegateHolder);

This is the incrementBalance function:

 function incrementBalance(mapping(address delegateTokenHolder => uint256 balance) storage balances, address delegateTokenHolder) internal {
        unchecked {
            ++balances[delegateTokenHolder];
        } // Infeasible that this will overflow
    }

This issue

Tools Used

Manual review

Recommended Mitigation Steps

Increment the balance by the amount of tokens received rather than the tokens sent.

Alternatively, Delegate could not accept fee-on-transfer tokens and only allow whitelisted tokens as a temporary countermeasure.

Assessed type

ERC20

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Out of scope

[GalloDaSballo]

Known