code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20MultiVotes.sol#L303-L306 https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Gauges.sol#L508-L511 https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L323-L330

Vulnerability details

Impact

Tokens won't be compatible with some protocols and will end up stranded

Vulnerability description

transferFrom from ERC20Gauges, ERC20MultiVotes and ERC20Boost attempts to use allowance even when spender = from. This breaks compatibility with a large number of protocol who opt to use the transferFrom method all the time (pull only) instead of using both transfer and transferFrom (push and pull). The ERC20 standard only does an allowance check when spender != from. The result of this difference will likely result in tokens becoming irreversibly stranded across different protocols.

Proof of Concept

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Gauges.sol#L508-L511 https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20MultiVotes.sol#L303-L306 https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-20/ERC20Boost.sol#L323-L330

The transferFrom methods shown above always uses allowance even if spender = from.

Tools Used

Manual review and good memory of other similar issues

Recommended Mitigation Steps

Modify the transferFrom functions to default on simple transfer if from == sender. Example for ERC20MultiVotes::transferFrom:

    function transferFrom(address from, address to, uint256 amount) public virtual override returns (bool) {
        _decrementVotesUntilFree(from, amount);
        if (from != msg.sender) {
            return super.transferFrom(from, to, amount);
        }
        return super.transfer(to, amount);
    }