fix(StakeVault): make unstaking actually work

[0x-r4bbit]

Unstaking didn't actually work because it was using transferFrom() on the StakeVault with the from address being the vault itself. This would result in an approval error because the vault isn't creating any approvals to spend its own funds.

The solution is to use transfer instead, or rather safeTransfer using SafeERC20 utilities.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

• Added natspec comments?
• Ran forge snapshot?
• Ran pnpm lint?
• Ran forge test?
• Ran pnpm verify?

[3esmit]

Indeed, it should use transfer instead of transferFrom in the unstake method.

Why its using these safeTransfer and safeTransferFrom?

[0x-r4bbit]

I've used safeTransfer() over transfer() in case we want to support staking of other ERC20 tokens as well. Given some ERC20 tokens don't behave exactly per spec, safeTransfer is the better choice here.

If we're 100% certain this will only ever be used with SNT that we have designed, then we can also use plain transfer instead.

[3esmit]

If the users trust a token that misbehave ERC20 spec, thats user responsability.
If the user stake whatever bad token they want, their staking would be worthless.

I am unsure how that would work, but essentially staking should be over a token that is trusted by the community, in a social setting. If we are willing to babysit the user for this, than why arent we doing it for everything else?

I don't think that using safeTransferFrom and safeTransfer would cause any problem, but I think is a fix that we don't need, or at least, is not part of making unstaking work.

[0x-r4bbit]

I don't think that using safeTransferFrom and safeTransfer would cause any problem, but I think is a fix that we don't need, or at least, is not part of making unstaking work.

Yes, very fair. I considered splitting this up into multiple commits. One that fixes the bug and another one that introduces safeTransfer() for the reason mentioned above. But then I decided to do it in one go.

I will undo this change and rely on plain transfer() instead.

Just so this is on the record, unless we specify the constraints and limits of the system clearly, this would probably be one of the things security auditors will point out as a high or mid.

[0x-r4bbit]

@3esmit I've updated the PR with the following:

• I've removed SafeERC20 usage
• safeTransfer and safeTransferFrom are now plain transfer and transferFrom calls
• Return values of those calls are checked and cause a revert if false
• Added a test with a broken erc20 that would always return false on transfers

Let me know if this does the job for you