The Fee Manager NFT Shares can be drained

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L274

Vulnerability details

Impact

Loss of protocol funds

Proof of Concept

The users having positions at the Power Farms can withdraw their shares by calling manuallyWithdrawShares.

The function is below;

Contract: PendlePowerManager.sol

274:     function manuallyWithdrawShares(
275:         uint256 _keyId,
276:         uint256 _withdrawShares
277:     )
278:         external
279:         updatePools
280:         onlyKeyOwner(_keyId)
281:     {
282:         uint256 wiseLendingNFT = farmingKeys[
283:             _keyId
284:         ];
285: 
286:         _manuallyWithdrawShares(
287:             wiseLendingNFT,
288:             _withdrawShares
289:         );
290: 
291:         if (_checkDebtRatio(wiseLendingNFT) == false) {
292:             revert DebtRatioTooHigh();
293:         }
294: 
295:         emit ManualWithdrawShares(
296:             _keyId,
297:             wiseLendingNFT,
298:             _withdrawShares,
299:             block.timestamp
300:         );
301:     }

Accordingly, the users should pass their _keyId and amount of shares that they want to withdraw.

The updatePools modifier syncs the pools and the onlyKeyOwner modifier validates the caller being the key owner.

Accordingly, the function continues with converting PENDLE_CHILD shares to tokens by calling _manuallyWithdrawShares and the shares are cashed out accordingly in Wiselending Contract as below.

Contract: PendlePowerFarm.sol

157:     function _manuallyWithdrawShares(
158:         uint256 _nftId,
159:         uint256 _withdrawShares
160:     )
161:         internal
162:     {
163:         uint256 withdrawAmount = WISE_LENDING.cashoutAmount(
164:             PENDLE_CHILD,
165:             _withdrawShares
166:         );
167: 
168:         withdrawAmount = WISE_LENDING.withdrawExactShares(
169:             _nftId,
170:             PENDLE_CHILD,
171:             _withdrawShares
172:         );
173: 
174:         _safeTransfer(
175:             PENDLE_CHILD,
176:             msg.sender,
177:             withdrawAmount
178:         );
179:     }

L: 168 withdraws the tokens using shares and the tokens are transferred afterward.

Finally, in the PendlePowerManager.manuallyWithdrawShares function, a debt ratio is checked, accordingly, the call is finalized and the caller receives the funds.

---

Scenario;

1. MEV Bot calls manuallyWithdrawShares with _keyId = 0
2. The pools are synched by the updatePools modifier
3. onlyKeyOwner modifier validates the keyId ownership as below;

Contract: MinterReserver.sol

32:     modifier onlyKeyOwner(
33:         uint256 _keyId
34:     ) {
35:         _onlyKeyOwner(
36:             _keyId
37:         );
38:         _;
39:     }
40: 
41:     function _onlyKeyOwner(
42:         uint256 _keyId
43:     )
44:         private
45:         view
46:     {
47:         require(
48: >           isOwner(
49:                 _keyId,
50:                 msg.sender
51:             ) == true
52:         );
53:     }
54: 

And isOwner function is below;

Contract: MinterReserver.sol

 95: 
 96:     function isOwner(
 97:         uint256 _keyId,
 98:         address _owner
 99:     )
100:         public
101:         view
102:         returns (bool)
103:     {
104: >        if (reservedKeys[_owner] == _keyId) {
105:             return true;
106:         }
107: 
108:         if (FARMS_NFTS.ownerOf(_keyId) == _owner) {
109:             return true;
110:         }
111: 
112:         return false;
113:     }

4. Since the MEV Bot doesn´t have any reservedKeys, L: 104 returns true for the keyId = 0. So the onlyKeyOwner modifier would be bypassed.
5. Continuing from the function flow below;

   
   Contract: PendlePowerFarm.sol

157: function _manuallyWithdrawShares( 158: uint256 _nftId, 159: uint256 _withdrawShares 160: ) 161: internal 162: { 163: > uint256 withdrawAmount = WISE_LENDING.cashoutAmount( 164: PENDLE_CHILD, 165: _withdrawShares 166: ); 167: 168: > withdrawAmount = WISE_LENDING.withdrawExactShares( 169: _nftId, 170: PENDLE_CHILD, 171: _withdrawShares 172: ); 173: 174: _safeTransfer( 175: PENDLE_CHILD, 176: msg.sender, 177: withdrawAmount 178: ); 179: }

L: 163 calls `Wiselending` contract to calculate the token amount as:
```solidity
Contract: MainHelper.sol

100:     {
101:         return _shares
102:             * lendingPoolData[_poolToken].pseudoTotalPool
103:             / lendingPoolData[_poolToken].totalDepositShares - 1;
104:     }

L: 168 calls Wiselending contract´s withdrawExactShares with NFTid being ZERO. That´s the Id of FEE_MANAGER_NFT And FEE_MANAGER_NFT also claims the fees in the same fashion;

Contract: FeeManager.sol

644:         uint256 tokenAmount = WISE_LENDING.withdrawExactShares(
645:  >          FEE_MANAGER_NFT, //@audit 0 id
646:             _poolToken,
647:             shares
648:         );
649: 

6. Since the FEE_MANAGER_NFT doesn´t have any debt, the MEV Bot receives the tokens that the FEE_MANAGER_NFT is allocated.

Tools Used

Manual Review

Recommended Mitigation Steps

Do not allow manuallyWithdrawShares to be called with Id = 0.

Assessed type

Access Control

[c4-pre-sort]

GalloDaSballo marked the issue as sufficient quality report

[c4-pre-sort]

GalloDaSballo marked the issue as remove high or low quality report

[GalloDaSballo]

This line does pass:

if (reservedKeys[_owner] == _keyId) {

I'm not convinced this would yield any value:

        withdrawAmount = WISE_LENDING.withdrawExactShares(
            _nftId,
            PENDLE_CHILD,
            _withdrawShares
        );

[vm06007]

This is invalid report and can be dismissed. It is not possible to call manuallyWithdrawShares on PendlePowerFarm contract with _keyId -> this will always revert with NotOwner() error message.

This is because whoever submitted it didn't pay attention that when powerFarm will call withdrawExactShares on WISE_LENDING contract then WISE_LENDING contract will perform a check for nftId parameter (in this case 0)

see -> _checkOwnerPosition() function which will make sure that nftId in this case 0 must be owned by the caller (in this case it requires for nftId 0 to be inside the powerFarm contract, but it is in feeManager contract.

You can run a test and see for yourself.

see here: https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/WiseLowLevelHelper.sol#L434-L445

this function makes sure that nftId 0 must be owner by the caller, and feeManagerNFT (0) is not owned by PowerFarm so the call will fail.

Dismissed.

[c4-pre-sort]

GalloDaSballo marked the issue as insufficient quality report

[c4-judge]

trust1995 marked the issue as unsatisfactory: Invalid