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#L210-L252

Vulnerability details

Impact

Loss of user funds that enter the power farms

Proof of Concept

Users who want to leverage their gains enter the power farms by calling;

enterFarm - The NFT id is assigned by calling _getWiseLendingNFT

enterFarmETH - The NFT id is assigned by calling _getWiseLendingNFT

And they call exitFarm function when they want to exit the power farms and withdraw their funds from the farms.

Bob is a malicious actor and he doesn´t possess any Position NFT or Power Farm NFT or any reservations. In addition, for the simplicity, there´s no available Power Farm NFT for the latter assignment (availableNFTCount == 0 condition)

He simply calls exitFarm with _keyId = 0

For the call trace, exitFarm logic is as follows;

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

Supplied keyId is validated by onlyKeyOwner(_keyId) modifier call;
```
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: }


And `isOwner` function works as follows:

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

Since Bob doesn´t have a reserved key, and the input keyId = 0, L: 104 returns to be True, and validation is fulfilled.
At the exitFarm function, L: 219 parses the wiseLendingNFT from the farmingKeys mapping which is 0 due to supplied keyId doesn´t exist.
L: 227 to L:228 returns true and resets the mapping (which is actually the same as the existing condition)
L: 235 to L: 237 increments the availableNFTCount counter and sets it to 0 for the latter assignment. Accordingly, the mapping status is as follows;
```
availableNFTs[1] = 0
```
L: 239 calls the _closingPosition function and it results in WISE_LENDING.paybackExactShares return 1 due to the below as per the call trace;
```
Contract: MainHelper.sol
```

114: function paybackAmount( 115: address _poolToken, 116: uint256 _shares 117: ) 118: public 119: view 120: returns (uint256) 121: { 122: uint256 product = _shares //@audit product results in being 0 123: * borrowPoolData[_poolToken].pseudoTotalBorrowAmount; 124: 125: uint256 totalBorrowShares = borrowPoolData[_poolToken].totalBorrowShares; 126: 127: return product / totalBorrowShares + 1; //@audit Here results in 1 128: }

8. And Bob exits the `exitFarm` call without any revert.
9. Bob does the same call immediately. Call `exitFarm` without holding any protocol NFTs or reservations.

Accordingly, the `availableNFTs` mapping status becomes as below;

```solidity
availableNFTs[1] = 0
availableNFTs[2[ = 0

Alice wants to enter the power farms and she calls enterFarm without holding any position/power farm NFT or reservations and pays 20 WETH.

Here´s the function logic to trace the call;

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

At L: 107, her wiselendingNFT is assigned by calling _getWiseLendingNFT which is as follows;


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

Since there are available NFTs in the system, L: 205 assigns Alice availableNFTs[2] = 0 and decrements the counter.

Alice's WETH is transferred to the contract between L:109 to L:114
A position is opened by internally calling _openPosition between L:116 to L:122
A keyId is generated by internally calling _reserveKey

Accordingly, this keyId is used in reservedKeys and farmingKeys mappings by registering as;

Contract: MinterReserver.sol

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

The user can manage their position by this keyId.

Bob immediately calls enterFarmafter Alice back running her with min WETH as per the declaration.
The system assigns the same wiselendingNFT id to Bob being 0 == same as Alice´s wiselendingNFT. And there would be no more availableNFTs left.
As per enterFarm's logic below happens;
```
Contract: PendlePowerManager.sol
```

109: _safeTransferFrom( //@audit 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 minimum as per the declaration settings.

The function flow becomes 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.

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

---

Since Bob sees this one as a 100% working attack on the ETH main net, he continues to call `exitFarm` with different addresses without holding any protocol NFTs or reservations.

For the ETH main net, he inflates the `availableNFTs` mapping and makes the status as below;
```solidity
availableNFTs[1] = 0
availableNFTs[2] = 0
availableNFTs[3] = 0
availableNFTs[4] = 0
availableNFTs[5] = 0
.
.
.
availableNFTs[1000] = 0

So Bob can back run the users while holding Wiselending 0 and call exitFarm

He does the similar in Arbitrum listening to FarmEntry(keyId = 0) events to get into the attack.

Bob can exploit every single user entering Farms in ETH main net any many in Arbitrum by simply inflating the availableNFTs mapping with 0 id, and calling enterFarm & exitFarm with querying his Wiselending NFT will be assigned as 0.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend not allowing exitFarm call with 0 id input.

Assessed type

Access Control

GalloDaSballo commented 6 months ago

Same pre-condition as #193 so similar doubt on the second part

vm06007 commented 6 months ago

I'm not sure what the submitter was thinking, but it should be acknowledged that from the first step description of Bob calling exitFarm with keyId 0 such operation is not possible by design. If you try to call this you will get ERC721: invalid token ID as most optimistic scenario especially because keys are always burned and only can be incremented over time.

Secondly, even if that would be possible assume keyId exists, then this would still revert because keyId 0 can only point to wiseLending nftId 0 and that nft is not part of the FARM! therefor there is no opened position for wiseLending nftId 0 and entire transaction would revert when _closePosition() will be triggered with wiseLending nftId 0

The claim here is not sufficient, if author wants to prove his point please provide POC file for foundry to run the scenario. Feel free to try this also on these test contracts:

https://arbiscan.io/address/0xae6396577fa2596cd71e822c85ea615fbd74f879 https://arbiscan.io/address/0x9690eD5006e4B7cdB591B794C339Bd3c18E88e30

vm06007 commented 6 months ago

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

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

Attacker can theft almost all users that enter the farms by abusing zero id #170