Lines of code

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

Vulnerability details

Impact

The StakerVault contract doesn't protect against the "Multiple Withdrawal Attack" where:

Alice approve()s amount X for Bob
Alice decides that X was not enough, so approve()s X+N for Bob instead, intending for X+N to replace X
While the second approve() call is still in the mempool, Bob front-runs the approval with a transferFrom() for the original X amount by providing a high gas fee
When the second approve() call is added to a block, Bob transfers the new X+N amount

Bob was able to get 2X+N rather than X+N

This was found to be of Medium risk in a prior contest

Proof of Concept

File: protocol/contracts/StakerVault.sol   #1

185       function approve(address spender, uint256 amount) external override notPaused returns (bool) {
186           _allowances[msg.sender][spender] = amount;
187           emit Approval(msg.sender, spender, amount);
188           return true;
189       }

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

There are no checks that the existing value is zero

Tools Used

Code inspection

Recommended Mitigation Steps

One way to handle the issue is to add require(0 == allowance[msg.sender][spender] || 0 == amount), which will prevent EOA accounts from hitting the problem. For contract accounts, OpenZeppelin's ERC20 has functions increaseAllowance() and decreaseAllowance() which use allowance() calls in order to atomically change balances

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