`Incentivizer.sol` Tokens with fee on transfer are not supported

[code423n4]

Handle

WatchPug

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() or transferFrom().

In the current implementation, Incentivizer.sol#create() assumes that the received amount is the same as the transfer amount, and uses it to calculate reward amounts.

As a result, in claim(), later users may not be able to successfully claim their rewards, as it may revert at L223 for insufficient balance.

https://github.com/code-423n4/2021-12-perennial/blob/fd7c38823833a51ae0c6ae3856a3d93a7309c0e4/protocol/contracts/incentivizer/Incentivizer.sol#L59-L93

function create(ProgramInfo calldata info)
nonReentrant
isProduct(info.product)
notPaused
external returns (uint256) {
    bool protocolOwned = msg.sender == factory().owner();

    if (_registry[info.product].length() >= programsPerProduct) revert IncentivizerTooManyProgramsError();
    if (!protocolOwned && msg.sender != factory().owner(info.product))
        revert NotProductOwnerError(msg.sender, info.product);

    uint256 programId = _programInfos.length;
    (ProgramInfo memory programInfo, UFixed18 programFee) = ProgramInfoLib.create(fee, info);

    _programInfos.push(programInfo);
    _programs[programId].initialize(programInfo, protocolOwned);
    _registry[info.product].add(programId);
    fees[info.token] = fees[info.token].add(programFee);

    info.token.pull(msg.sender, info.amount.sum());

    emit ProgramCreated(
        programId,
        programInfo.product,
        programInfo.token,
        programInfo.amount.maker,
        programInfo.amount.taker,
        programInfo.start,
        programInfo.duration,
        programInfo.grace,
        programFee
    );

    return programId;
}

https://github.com/code-423n4/2021-12-perennial/blob/fd7c38823833a51ae0c6ae3856a3d93a7309c0e4/protocol/contracts/incentivizer/Incentivizer.sol#L216-L226

function claimInternal(address account, uint256 programId) private {
    Program storage program = _programs[programId];
    ProgramInfo memory programInfo = _programInfos[programId];

    program.settle(programInfo, account);
    UFixed18 claimedAmount = program.claim(account);

    programInfo.token.push(account, claimedAmount);

    emit Claim(account, programId, claimedAmount);
}

Recommendation

Consider comparing before and after balance to get the actual transferred amount.

[kbrizzle]

We generally do not support tokens that take a fee on transfer or any other non-standard ERC20s. Further, the incentive programs can only be started by either the Perennial owner / DAO or the product's owner / DAO, so it's unlikely they would attempt to use a non-standard token given the constraint.

The additional check proposed is quite lightweight though, so it's worth adding as a failsafe to prevent this from occurring.

[kbrizzle]

Update: after looking through this more deeply, it does not look like this additional check is worth the considerable gas-complexity overhead. Further, UniswapV3's canonical Staker contract, which has a similar architecture, does not implement a similar check.

We're going to propose this be moved to 1 (Low) severity given the unlikelihood an unsupported token being used by a product owner.

We intend to resolve this by adding more robust documentation on what tokens the Incentivizer supports in its programs.

[GalloDaSballo]

@kbrizzle Please note that UniV3Staker is not audited https://twitter.com/litocoen/status/1471569495424745477

[GalloDaSballo]

First of all I agree with the warden is saying that the code will have accounting issues with feeOnTransfer (as well as malicious tokens that have their transfer altered).

In terms of impact, the incentives have to be claimed by specifying the programId meaning that no real DOS can ever happen (just don't call claim on the malicious program), additionally issues with accounting in one program with one token don't seem to impact additional programs.

The only exception I can think of would be for a custom token built in such a way where the fee can be added and removed, because the Incentivizer holds the balances of tokens, there can be a scenario where claiming a bounty would allow you to take tokens from another bounty.

I find this extremely unlikely, and limited in the damage it could cause (all losses have to be denominated in this malicious token)

Because this can be sidestepped so easily, I agree with a Low Severity

[kbrizzle]

The V3 staker was actually audited: it's listed here, but it doesn't look like the report was made public for some reason.

[GalloDaSballo]

Thank you for the correction

I checked their code, and if you set up a reward with a feeOnTransfer or any token that changes it's balance during the transfer, I believe the accounting of their contract will break as well

Similarly to this finding, the only bounty that will break is the one setup in that token so I don't believe this will cause any risk beside the inability of claiming tokens from those bounties

[GalloDaSballo]

Code I checked: https://etherscan.io/address/0xe34139463bA50bD61336E0c446Bd8C0867c6fE65#code#F1#L117