code-423n4 / 2022-12-tigris-findings

8 stars 4 forks source link

ERC20 Tokens with fee on transfer are not supported #613

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/GovNFT.sol#L287-L294 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/BondNFT.sol#L216-L227 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L61-L76 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L84-L92 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/StableVault.sol#L44-L51 https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L651-L653

Vulnerability details

ERC20 Tokens with fee on transfer are not supported

Vulnerability details

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

GovNFT.sol#distribute() assumes that the received amount is the same as the transfer amount, and uses it to calculate rewards. While the actual transferred amount can be lower for those tokens.

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/GovNFT.sol#L287-L294

        try IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount) {
            accRewardsPerNFT[_tigAsset] += _amount/totalSupply(); //@audit amount can be less than expected

Same happens on other contracts and functions

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/BondNFT.sol#L216-L227

accRewardsPerShare[_tigAsset][aEpoch] can be less than expected

        IERC20(_tigAsset).transferFrom(_msgSender(), address(this), _amount); 
        unchecked {
            uint aEpoch = block.timestamp / DAY; 
            if (aEpoch > epoch[_tigAsset]) {
                for (uint i=epoch[_tigAsset]; i<aEpoch; i++) { 
                    epoch[_tigAsset] += 1; 
                    accRewardsPerShare[_tigAsset][i+1] = accRewardsPerShare[_tigAsset][i];
                }
            }
            accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];//@audit uses amount for calculating rewards, can be actually less
        }
        emit Distribution(_tigAsset, _amount);

https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L61-L76

        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        totalLocked[_asset] += _amount; //@audit uses amount for calculating locked amount

        bondNFT.createLock( _asset, _amount, _period, msg.sender); //@audit bad amount
    }

Same while extending locked amount extendLock https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Lock.sol#L84-L92

While mintingFor https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/StableVault.sol#L44-L51

In the deposit https://github.com/code-423n4/2022-12-tigris/blob/0cb05a462e78c4470662e9d9a4f9ab587f266bb5/contracts/Trading.sol#L651-L653

Recommendation

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

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #522

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-12-tigris-findings/issues/662

GalloDaSballo commented 1 year ago

L

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-b

Simon-Busch commented 1 year ago

Removed duplicate-522 as requested by @GalloDaSballo