code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/2a5664d35cde5b036074edef3c1369b984d10010/protocol/contracts/StakerVault.sol#L381-L383

Vulnerability details

Impact

When calling StakerVault.unstakeFor(), the user's balance is decremented by the value of unstaked. The withdrawal transfer occurs prior to calculating the amount of unstaked. Given that token is an ERC20Upgradeable, it is within reason that this token will be upgraded in the future and will make a call to the receiver as is the standard for ERC777 or ERC1155 tokens.

        uint256 oldBal = IERC20(token).balanceOf(address(this));
        IERC20(token).safeTransfer(dst, amount);
        uint256 unstaked = oldBal.uncheckedSub(IERC20(token).balanceOf(address(this)));
        balances[src] -= unstaked;

The vulnerability lies in the fact that unstaked is calculated based on the contract's token balances before and after the transfer. If the token were to transfer control flow to the receiver, the receiver could simply stake the withdrawn value, effectively printing money.

Proof of Concept

Steps for exploit

User stakes 100 ether of token
User unstakes all 100 ether
If the token provides control to the receiver, the user will receive the tokens via safeTransfer() and will immediately call stake() with the same 100 ether.
unstaked will calculate to 0 since the contract's token balance will remain unchanged.
User's balance has increased to 200 ether.
User can repeat as many times as desired.

Tools Used

Manual review

Recommended Mitigation Steps

Do not check the contract's token balance in order to calculate the value of unstaked. Simply subtract the amount that was sent via safeTransfer().

chase-manning commented 2 years ago

We do not support tokens that offer reentrancy opportunities

GalloDaSballo commented 2 years ago

For a discussion similar to #19 , while impact is minimal, I believe the finding to have validity in that the code doesn't respect CEI and doesn't have a nonReentrant guard.

For those reasons I believe QA to be more appropriate

code-423n4 / 2022-05-backd-findings