code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L14-L27

Vulnerability details

Impact

The DAO may inadvertently cause the batched sendWithdrawalRequests() to fail, incurring enough delays, that the withdrawals miss the 12-hr Tokemak window, and thus miss out on expected returns.

Proof of Concept

As time goes on the DAO may add new contracts to BatchRequests.contracts. Basic checks by calling canBatchContracts() will pass, but since it involves double the number of external calls, sendWithdrawalRequests() may run out of gas and revert:

File: src/contracts/BatchRequests.sol   #1

14       function sendWithdrawalRequests() external {
15           uint256 contractsLength = contracts.length;
16           for (uint256 i; i < contractsLength; ) {
17               if (
18                   contracts[i] != address(0) &&
19                   IStaking(contracts[i]).canBatchTransactions()
20               ) {
21                   IStaking(contracts[i]).sendWithdrawalRequests();
22               }
23               unchecked {
24                   ++i;
25               }
26           }
27       }

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/BatchRequests.sol#L14-L27

The DAO itself has at least a three-week delay for its governance process, so removeAddress() can't be used to rectify the situation. The only way for things to work within the 12-hour Tokemak windows is: 1) User submitting the call to sendWithdrawalRequests() was awake and watching it happen when it was done (i.e. wasn't asleep for 8 hours with the call having failed) 2) The user is aware that there's a 12-hour Tokemak deadline approaching 3) The user is in direct contact with the responsible DAO members and or devs 4) The parties involved quickly figure out that the DAO process is too slow, and properly identify the contracts that need withdrawal requests to be done

Without all four steps above, there will be too many delays to have all user's funds properly re-submitted in the correct Tokemak window

Tools Used

Code inspection

Recommended Mitigation Steps

Add an offset and a count to the sendWithdrawalRequests() function's parameters, which would allow the splitting of the batch into multiple, smaller parts

toshiSat commented 2 years ago

sponsor disputed. We are using gelato to manually do this within a 6 hour window. So we don't have to worry about people being asleep for this. Also it's an external call anyone can call.

JasoonS commented 2 years ago

This is definitely a concern, will make informational.

Gelato/Keeper etc can also have issues, it is not completely impossible.

code-423n4 / 2022-06-yieldy-findings