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

0 stars 0 forks source link

Arbitrage on `stake()` #243

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Issue: there is a huge arb opportunity for people who deposit 1 block before the rebase()

Consequences: then they can call instantUnstakeReserve or instantUnstakeCurve to unstake the staked amount, in this way the profit that needs to be distributed on the next rebase increases, he also messes up the rewards for the other holders as the instantUnstakeReserve does not burn the YIELD_TOKEN. Even if there is a fee on the instantUnstakeReserve, there is still a chance for profit.

Affected Code

File: Staking.sol
406:     function stake(uint256 _amount, address _recipient) public { // @audit-info [HIGH] 
407:         // if override staking, then don't allow stake
408:         require(!isStakingPaused, "Staking is paused");
409:         // amount must be non zero
410:         require(_amount > 0, "Must have valid amount");
411: 
412:         uint256 yieldyTotalSupply = IYieldy(YIELDY_TOKEN).totalSupply();
413: 
414:         // Don't rebase unless tokens are already staked or could get locked out of staking
415:         if (yieldyTotalSupply > 0) {
416:             rebase();
417:         }
418: 
419:         IERC20Upgradeable(STAKING_TOKEN).safeTransferFrom(
420:             msg.sender,
421:             address(this),
422:             _amount
423:         );
424: 
425:         Claim storage info = warmUpInfo[_recipient];
426: 
427:         // if claim is available then auto claim tokens
428:         if (_isClaimAvailable(_recipient)) {
429:             claim(_recipient);
430:         }
431: 
432:         _depositToTokemak(_amount);
433: 
434:         // skip adding to warmup contract if period is 0
435:         if (warmUpPeriod == 0) {
436:             IYieldy(YIELDY_TOKEN).mint(_recipient, _amount);
437:         } else {
438:             // create a claim and mint tokens so a user can claim them once warm up has passed
439:             warmUpInfo[_recipient] = Claim({
440:                 amount: info.amount + _amount,
441:                 credits: info.credits +
442:                     IYieldy(YIELDY_TOKEN).creditsForTokenBalance(_amount),
443:                 expiry: epoch.number + warmUpPeriod
444:             });
445: 
446:             IYieldy(YIELDY_TOKEN).mint(address(this), _amount);
447:         }
448: 
449:         sendWithdrawalRequests();
450:     }

Mitigations

Burn the YIELD_TOKEN amount in the instantUnstakeReserve

0xean commented 2 years ago

Yes, the fee on instant Unstake needs to be set high enough to make this not profitable.

If a curve pool exists, then this does become possible to arb the rebase and something that should be fixed, potentially with not allowing the warm up period to be violated for instant unstaking (through curve at the very least).

I would qualify this as Medium severity, and leaking value.

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.
JasoonS commented 2 years ago

I took another look, medium seems reasonable too.