code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/compound_rari_fork/Comptroller.sol#L767-L809

Vulnerability details

Impact

addNFTCollateral will be failed if there are too many Uniswap V3 LP Vault added and user has deposit into too many pools. The gas usage requirement for executing addNFTCollateral will exceed maximum limit for a particular chain.

Proof of Concept

First take a look at looping in the Duality's addNFTCollateral function at https://github.com/code-423n4/2022-04-dualityfocus/blob/f21ef7708c9335ee1996142e2581cb8714a525c9/contracts/compound_rari_fork/Comptroller.sol#L767-L809

uint256 userTokensLength = uniV3LpVault.getUserTokensLength(account);
for (uint256 i = 0; i < userTokensLength; i++) {

Then let take a look at pancakeswap masterchef contract. https://bscscan.com/address/0x73feaa1ee314f8c655e354234017be2193c9e24e

    function massUpdatePools() public {
        uint256 length = poolInfo.length;
        for (uint256 pid = 0; pid < length; ++pid) {
            updatePool(pid);
        }
    }

In massUpdatePools function, it is doing similar to addNFTCollateral in term of looping over pools.

If you try to execute massUpdatePools function of pancakeswap, you will see error with extremely high amount of gas (around 0.37 BNB). This indicate that this function consume too much gas than the limit for a particular chain due to pools looping.

Currently pancakeswap has 523 pools

Since addNFTCollateral is much more complex than pancakeswap masterchef massUpdatePools, just 400 pools may cause this problem.

Tools Used

Can be found by reading code

Recommended Mitigation Steps

Deploy contracts
Add around 400 pools
Let user deposit to all 400 pools one by one
At some time, user may not able to deposit any more due to gas usage exceed maximum limit for a particular chain.

0xdramaone commented 2 years ago

Acknowledged, but disagree this is an issue since we have a max deposit of NFT positions which will never approach this limit

jack-the-pug commented 2 years ago

Not a valid issue as _userTokensMax exits.

code-423n4 / 2022-04-dualityfocus-findings