GovernanceWrappedERC20 isn't compatible with fee-on-transfer or rebasing tokens

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/4db573870aa4e1f40a3381cdd4ec006222e471fe/packages/contracts/src/token/ERC20/governance/GovernanceWrappedERC20.sol#L81-L86

Vulnerability details

Impact

Fee-on-transfer and rebasing tokens accounting will be wrong if used as the underlying token

Proof of Concept

function depositFor(
    address account,
    uint256 amount
) public override(IGovernanceWrappedERC20, ERC20WrapperUpgradeable) returns (bool) {
    return ERC20WrapperUpgradeable.depositFor(account, amount);
}

When depositing the user is credited amount, not the actual number of tokens received. This is problematic for fee-on-transfer tokens in which the contract will receive less than what was sent.

function withdrawTo(
    address account,
    uint256 amount
) public override(IGovernanceWrappedERC20, ERC20WrapperUpgradeable) returns (bool) {
    return ERC20WrapperUpgradeable.withdrawTo(account, amount);
}

Likewise when withdrawing it sends the amount burned but for rebasing tokens this amount should be greater since the rebasing token will have likely increased. The result is that tokens will be stuck in the wrapper and unrecoverable.

Tools Used

Manual Review

Recommended Mitigation Steps

Either make users aware that these tokens cannot be used or make accommodations so that they are supported.

[c4-judge]

0xean marked the issue as duplicate of #125

[c4-judge]

0xean changed the severity to QA (Quality Assurance)

[c4-judge]

0xean marked the issue as grade-c