Fee on transfer tokens can lead to incorrect approval

[code423n4]

Handle

hrkrshnn

Vulnerability details

Fee on transfer tokens can lead to incorrect approval

The createBasket function does not account for tokens with fee on transfer.

function createBasket(uint256 idNumber) external override returns (IBasket) {
    // ...
    for (uint256 i = 0; i < bProposal.weights.length; i++) {
        IERC20 token = IERC20(bProposal.tokens[i]);
        token.safeTransferFrom(msg.sender, address(this), bProposal.weights[i]);
        token.safeApprove(address(newBasket), bProposal.weights[i]);
    }
    // ...
}

The function safeTransferFrom may not transfer exactly bProposal.weights[i] amount of tokens, for tokens with a fee on transfer. This means that the safeApprove call in the next line would be approving more tokens than what was received, leading to accounting issues.

Recommended Mitigation Steps

It is recommended to find the balance of the current contract before and after the transferFrom to see how much tokens were received, and approve only what was received.

[frank-beard]

the protocol for now is only expected to work with defi safe, standard erc-20 tokens.

[GalloDaSballo]

This finding is similar to #206 , but in contrast to it, it shows a specific way to brick / grief the protocol, as per the docs:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

This is an hypothetical attack with stated assumptions

Will not mark as duplicate and will consider this as a valid finding as it shows a specific vulnerability when paired with feeOnTransfer tokens

Mitigation can be as simple as never using feeOnTransfer tokens