search

code-423n4 / 2024-02-wise-lending-findings

11 stars 8 forks source link

Assigning available NFTs in PendlePowerManager leads to position thefts #169

Closed c4-bot-7 closed 5 months ago

c4-bot-7 commented 6 months ago

Lines of code

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

Vulnerability details

Impact

Loss of user funds

Proof of Concept

The PowerFarmNFTs are vital for the protocol as the positions being held in the farms are represented by the ID of the PowerFarmNFT.

If the user wants to leverage their gains by entering Power Farms they call;

enterFarm - The NFT id is assigned by calling _getWiseLendingNFT

enterFarmETH - The NFT id is assigned by calling _getWiseLendingNFT

Let´s check the user flow to understand how the positions are handled.

Alice holds a Position NFT and she wants to leverage her gains by the farms.

  1. Alice calls enterFarm

enterFarm logic is below;

Contract: PendlePowerManager.sol

 96:     function enterFarm(
 97:         bool _isAave,
 98:         uint256 _amount,
 99:         uint256 _leverage,
100:         uint256 _allowedSpread
101:     )
102:         external
103:         isActive
104:         updatePools
105:         returns (uint256)
106:     {
107:         uint256 wiseLendingNFT = _getWiseLendingNFT();
108: 
109:         _safeTransferFrom(
110:             WETH_ADDRESS,
111:             msg.sender,
112:             address(this),
113:             _amount
114:         );
115: 
116:         _openPosition(
117:             _isAave,
118:             wiseLendingNFT,
119:             _amount,
120:             _leverage,
121:             _allowedSpread
122:         );
123: 
124:         uint256 keyId = _reserveKey(
125:             msg.sender,
126:             wiseLendingNFT
127:         );
128: 
129:         isAave[keyId] = _isAave;
130: 
131:         emit FarmEntry(
132:             keyId,
133:             wiseLendingNFT,
134:             _leverage,
135:             _amount,
136:             block.timestamp
137:         );
138: 
139:         return keyId;
140:     }

  1. L: 103 & L: 104 are the modifiers and they do the following; isActive : Checks whether the system has been shut down. updatePools : Checks whether there's a re-entrancy & syncs the pools.

  2. L: 107 calls _getWiseLendingNFT to obtain the WiseLendingNFTId. The function logic is as below;

    
Contract: PendlePowerManager.sol

185: function _getWiseLendingNFT() 186: internal 187: returns (uint256) 188: { 189: if (availableNFTCount == 0) { 190: 191: uint256 nftId = POSITION_NFT.mintPosition(); 192: 193: _registrationFarm( 194: nftId 195: ); 196: 197: POSITION_NFT.approve( 198: AAVE_HUB_ADDRESS, 199: nftId 200: ); 201: 202: return nftId; 203: } 204: 205: return availableNFTs[ 206: availableNFTCount-- 207: ]; 208: }

So if the system has available NFTs to assign, L:205 to L:207 assign the surplus NFT by assigning and decrementing the `availableNFTs` mapping.

Otherwise, if there's no availableNFT `(availableNFTCount == 0)`, the function checks the PositionNFT id.
If there's a position NFT of the user, it assigns `nftID` to `wiseLendingNFT`

OR

A new position NFT is minted to the user by `_mintPositionForUser` during the call at L:191.

4. Alice's WETH is transferred to the contract between L:109 to L:114
5. A position is opened by internally calling [_openPosition](https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PendlePowerFarm/PendlePowerFarm.sol#L185) between L:116 to L:122
6. A `keyId` is generated by internally calling [_reserveKey](https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PowerFarmNFTs/MinterReserver.sol#L77)

Accordingly, this `keyId` is used in `reservedKeys` and `farmingKeys` mappings by registering as;
```solidity
Contract: MinterReserver.sol

90:         reservedKeys[_userAddress] = keyId;
91:         farmingKeys[keyId] = _wiseLendingNFT;

The user can manage their position by this keyId.

When the time comes;

  1. Alice wants to exit her position and calls exitFarm

  2. Alice's ownership is validated in the onlyKeyOwner modifier. The function flow for this modifier is; onlyKeyOwner -> _onlyKeyOwner -> require(isOwner) and isOWner function is as below;

    Contract: MinterReserver.sol

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:     }

  3. After the keyId ownership is verified, we can have a deeper look at exitFarm function logic;

    
Contract: PendlePowerManager.sol

210: function exitFarm( 211: uint256 _keyId, 212: uint256 _allowedSpread, 213: bool _ethBack 214: ) 215: external 216: updatePools 217: onlyKeyOwner(_keyId) 218: { 219: uint256 wiseLendingNFT = farmingKeys[ 220: _keyId 221: ]; 222: 223: delete farmingKeys[ 224: _keyId 225: ]; 226: 227: if (reservedKeys[msg.sender] == _keyId) { 228: reservedKeys[msg.sender] = 0; 229: } else { 230: FARMS_NFTS.burnKey( 231: _keyId 232: ); 233: } 234: 235: availableNFTs[ 236: ++availableNFTCount 237: ] = wiseLendingNFT; 238: 239: _closingPosition( 240: isAave[_keyId], 241: wiseLendingNFT, 242: _allowedSpread, 243: _ethBack 244: ); 245: 246: emit FarmExit( 247: _keyId, 248: wiseLendingNFT, 249: _allowedSpread, 250: block.timestamp 251: ); 252: }

4. L:219 to L:221 parses the `wiseLendingNFT` from the `farmingKeys` mapping with the associated `keyId`
5. L:223 to L:225 deletes the `keyId` from the `farmingKeys` mapping
6. L:227 to L:228 deletes the `keyId` if found on the `reservedKeys` mappings of the sender
Else, it burns the `FarmNFT` of the associated `keyId` between L:230 to L:232

7. L:235 to L:237 assigns this NFT for latter minting and increments the `availableNFTCount` counter
8. `_closingPosition` is internally called and the rest goes by checking the position data, pricing, and finally transfer of the funds to Alice.

---

The above flow works fine but contains a bug where the users' `PowerFarmNFT` could be exploited due to the logic of `availableNFTs` mapping usage.

Let's go through the scenario.

1. Bob has a reserved `PositionNFT` of Id:5 and he wants to leverage his gains.
2. Bob enters the power farm by calling `enterFarm`
3. Bob's `wiselendingNFT` is enumerated to him by internally calling [_getWiseLendingNFT](https://github.com/code-423n4/2024-02-wise-lending/blob/79186b243d8553e66358c05497e5ccfd9488b5e2/contracts/PowerFarms/PendlePowerFarm/PendlePowerManager.sol#L185)  at L:107.
4. Since there's no available NFT in the system (availableNFTCount == 0 condition), his `wiselendingNFT` id becomes 5 as per his Position NFT due to the logic below;
```solidity
Contract: PendlePowerManager.sol

185:     function _getWiseLendingNFT()
186:         internal
187:         returns (uint256)
188:     {
189:         if (availableNFTCount == 0) {
190: 
191:     >      uint256 nftId = POSITION_NFT.mintPosition();
192: 
193:             _registrationFarm(
194:                 nftId
195:             );
196: 
197:             POSITION_NFT.approve(
198:                 AAVE_HUB_ADDRESS,
199:                 nftId
200:             );
201: 
202:             return nftId;
203:         }
204: 
205:         return availableNFTs[
206:             availableNFTCount--
207:         ];
208:     }

L:191 calls positionNFTs contract and it goes the flow like below; mintPosition -> _mintPositionForUser

Contract: PositionNFTs.sol

193:     function _mintPositionForUser(
194:         address _user
195:     )
196:         internal
197:         returns (uint256)
198:     {
199:  >      uint256 nftId = reserved[
200:             _user
201:         ];
202: 
203:         if (nftId > 0) {
204:             delete reserved[
205:                 _user
206:             ];
207: 
208:             totalReserved--
209: 
210:         } else {
211:             nftId = getNextExpectedId();
212:         }
213: 
214:         _mint(
215:             _user,
216:             nftId
217:         );
218: 
219:         return nftId;
220:     }

Since Bob has a reserved Position NFT, L:199 returns his reserved NFT id = 5.

  1. The funds are transferred from Bob and a position is opened between the Lines: 109 to 122
  2. A keyId is assigned to Bob by internally calling _reserveKey between the Lines: 124 to 127
  3. _reserveKey function logic is as follows; 
    
Contract: MinterReserver.sol

77: function _reserveKey( 78: address _userAddress, 79: uint256 _wiseLendingNFT 80: ) 81: internal 82: returns (uint256) 83: { 84: if (reservedKeys[_userAddress] > 0) { 85: revert AlreadyReserved(); 86: } 87: 88: uint256 keyId = _getNextReserveKey(); 89: 90: reservedKeys[_userAddress] = keyId; 91: farmingKeys[keyId] = _wiseLendingNFT; 92: 93: return keyId; 94: }


Since Bob doesn't have a reserved key in power farms, the system assigns him a new keyId by calling `_getNextReserveKey` at L:88
and it's functioning as below;
```solidity
Contract: MinterReserver.sol

70:     function _getNextReserveKey()
71:         internal
72:         returns (uint256)
73:     {
74:         return totalMinted + _incrementReserved();
75:     }

Let's assume that there are 2 minted positions and 3 totalReserved positions at this call. So it will return as 2 (totalMinted) + (3+1)(++totalReserved) = 6

Accordingly, Lines 90 & 91 will be;

90: reservedKeys[Bob] = 6
91: farmingKeys[6] = 5

after this call.

  1. Bob goes ahead and mints his Power Farm NFT by calling mintReserved And mintReserved internally calls _mintKeyForUser with the logic below; 
    
Contract: MinterReserver.sol

115: function _mintKeyForUser( 116: uint256 _keyId, 117: address _userAddress 118: ) 119: internal 120: returns (uint256) 121: { 122: if (_keyId == 0) { 123: revert InvalidKey(); 124: } 125: 126: > delete reservedKeys[ 127: > _userAddress 128: ]; 129: 130: FARMS_NFTS.mintKey( 131: _userAddress, 132: _keyId 133: ); 134: 135: totalMinted++; 136: totalReserved--; 137: 138: return _keyId; 139: }

L: 126 - 127 deletes the reserved key of Bob which was registered as 6 on the 7th step of this flow;
```solidity
90: reservedKeys[Bob] = 0
91: farmingKeys[6] = 5

L:135 increments the totalMinted and makes it 3

L:136 decrements the totalReservedand makes it 3

  1. Bob calls exitFarm as below; 
    
Contract: PendlePowerManager.sol

210: function exitFarm( 211: uint256 _keyId, 212: uint256 _allowedSpread, 213: bool _ethBack 214: ) 215: external 216: updatePools 217: onlyKeyOwner(_keyId) 218: { 219: uint256 wiseLendingNFT = farmingKeys[ 220: _keyId 221: ]; 222: 223: > delete farmingKeys[ 224: _keyId 225: ]; 226: 227: if (reservedKeys[msg.sender] == _keyId) { 228: reservedKeys[msg.sender] = 0; 229: } else { 230: > FARMS_NFTS.burnKey( 231: _keyId 232: ); 233: } 234: 235: availableNFTs[ 236: ++availableNFTCount 237: ] = wiseLendingNFT; 238: 239: _closingPosition( 240: isAave[_keyId], 241: wiseLendingNFT, 242: _allowedSpread, 243: _ethBack 244: ); 245: 246: emit FarmExit( 247: _keyId, 248: wiseLendingNFT, 249: _allowedSpread, 250: block.timestamp 251: ); 252: }


10. His `farmingKey` is deleted at L: 223
11. Since he doesn't have a reservedKey (it was deleted at the time of PowerFarmNFT minting)  the ELSE block burns his FarmNFT associated with the keyId.
12. `availableNFTs` mapping is set to Bob's wiseLendingNFT id by incrementing `availableNFTCount`
And organically it's the first NFT freed for later use and the mapping is registered like this;
```solidity
availableNFTs[1] = 5
  1. Alice wants to enter the Power Farms and she calls enterFarm with 20 WETH
  2. _getWiseLendingNFT assigns Alice the available NFT id left by Bob (5) and decrements the mapping afterwards while leaving no NFT available.
availableNFTs[1] = 5 // @audit Alice takes this before it's decremented

Alice's wiselendingNFT becomes 5.

  1. Regular functions are executed after this point, transfer of funds, opening position, reserving key.

_reserveKey function sets the mappings for Alice as below;

reservedKeys[Alice] = keyId = 7 == 3(totalMinted) + (3+1)(++totalReserved)
farmingKeys[7] = 5;
  1. Alice mints her Power Farm NFT and the new values are as follows; 
    totalMinted = 4;
totalReserved = 3;
  2. Bob re-enters the farm just after Alice by calling enterFarm but with min amount that the protocol allows him. It´s 3 ETH.
  3. _getWiseLendingNFT assigns the nftID as Bob's Position NFT id again due to there's no available Power Farm NFT id. Bob's wiselendingNFT becomes 5 too.
  4. As per enterFarm's logic below happens; 
    
Contract: PendlePowerManager.sol

109: _safeTransferFrom( //@audit 3 WETH is transferred from Bob. 110: WETH_ADDRESS, 111: msg.sender, 112: address(this), 113: _amount 114: ); 115: 116: _openPosition( // @audit 117: _isAave, 118: wiseLendingNFT, 119: _amount, 120: _leverage, 121: _allowedSpread 122: );


L: 116 initiates an `_openPosition` call with Alice's wiseLendingNFT and with the `_amount` being 3 WETH.

The function flow becomes as this for the user position;
`_openPosition` -> `_executeBalancerFlashLoan` -> `receiveFlashLoan` -> `_logicOpenPosition (due to initialAmount > 0)` -> Pendle Interactions to deposit SY -> `WISE_LENDING.depositExactAmount` -> `_handleDeposit` -> `_increasePositionLendingDeposit` -> `_addPositionTokenData`

No function in this call trace reverts and since `_increasePositionLendingDeposit` increments the nftID position by the new shares and the funds transferred successfully, Bob successfully enters the farm by adding his position on top of Alice´s shares.

19. Bob calls `exitFarm` with his assigned KeyID immediately and withdraws all shares and funds with the associated Wiselending NFT (5), including Alice´s funds. 

## Tools Used
Manual Review

## Recommended Mitigation Steps
We recommend not reserving used NFT´s for the latter assignment.

## Assessed type

Access Control
vm06007 commented 5 months ago
  • Bob has a reserved PositionNFT of Id:5 and he wants to leverage his gains.
  • Bob enters the power farm by calling enterFarm
  • Bob's wiselendingNFT is enumerated to him by internally calling _getWiseLendingNFT at L:107.
  • Since there's no available NFT in the system (availableNFTCount == 0 condition), his wiselendingNFT id becomes 5 as per his Position NFT due to the logic below;

author of this finding makes a critical assumption and puts wrong statement here without really understanding the code. here are the false assumptions:

1) when Bob enters the farm by calling enterFarm Bob will get keyId 1 that would point to wiseLending nftId 6, not 5 as was claimed, this is because wiseLending nftId 5 is already reserved for Bob and it is powerFarm that calls to wiseLending to mint a new NFT not Bob directly. 2) what will happen is that powerFarm will request a new wiseLending nftId to be minted into self and that is the one that is going to be assigned to Bob through keyId, Bob cannot use wiseLending nftId 5 in the farm.

later author of this "finding" continues false path of saying that _mintPositionForUser will be used to mint nft for Bob, but if you look closely to the code it will mint nft into the FARM and it will be nftId 6

the flaw in submiters "finding" is also in saying that

L:191 calls positionNFTs contract and it goes the flow like below; mintPosition -> _mintPositionForUser here the paramter will be powerFarm address not Bobs address is passed when calling mintPositionForUser

vm06007 commented 5 months ago

based on false assumptions provided in the middle of the finding the rest can be easily dismissed and marked as invalid finding, otherwise feel free to provide POC working foundry file showing your "finding" proof.

c4-pre-sort commented 5 months ago

GalloDaSballo marked the issue as insufficient quality report

c4-judge commented 5 months ago

trust1995 marked the issue as unsatisfactory: Insufficient proof