First Flight #4: Boss Bridge

Summary

In the constructor of L1BossBridge contract is allowed the bridge to move tokens out of the vault to facilitate withdrawals through the IERC20 approve function. The approved amount of tokens is equal to type(uint256).max that is called unlimited approve.

Vulnerability Details

The amount that is approved in the constructor of L1BossBridge contract is the maximum value of uint256. This amount of approved tokens is vulnerable because it gives the approved contract or address the ability to transfer any amount of tokens from the approver's account at any time. This can lead to unexpected loss of funds if the approved contract or address is compromised or behaves maliciously.

Impact

Let's consider the following scenario that shows why it's risky to approve an unlimited amount of tokens to be spent by a contract. If a signer is compromised or acts maliciously, they can drain all the tokens from the vault.

1. Alice is the owner of the L1BossBridge contract and Bob is a malicious actor.

2. Alice calls setSigner(Bob, true) to set Bob as a signer.

3. Alice deposits tokens into the contract by calling depositTokensToL2(Alice, L2Recipient, amount).

4. The tokens are transferred from Alice to the vault contract.

5. Now, Bob, being a signer, can create a valid signature for any withdrawal request. Bob calls withdrawTokensToL1(Bob, amount, v, r, s) with a valid signature, where v, r, and s are components of a signature that Bob created.

6. The withdrawTokensToL1 function calls sendToL1 with the encoded call to IERC20.transferFrom(address(vault), Bob, amount).

7. Since the vault contract has approved the L1BossBridge contract to spend an unlimited amount of tokens, the transferFrom call succeeds, and Bob is able to withdraw all the tokens from the vault to his own address.

Also, it is a good practice to check the returned value from the external call like approveTo. If the approveTo function fails and does not return true, it means that the allowance was not set correctly. Since the return value of this function is not checked in the L1BossBridge constructor, the contract deployment will proceed as if the allowance was set correctly. This could lead to serious issues later on when the L1BossBridge contract tries to transfer tokens from the L1Vault. If the allowance was not set correctly, these transfer operations will fail, but the contract has no way of knowing this in advance because the return value of the approveTo function was not checked.

Tools Used

VS Code, Foundry

Recommendations

Instead of approving an unlimited amount, approve only the amount that is needed for each operation. This would require changing the approval mechanism in the L1BossBridge contract. But it should be taken into account the important note in the documentation about the use of the approve function:

"Beware that changing an allowance with this method brings the risk that someone may use both the old and the new allowance by unfortunate transaction ordering. One possible solution to mitigate this race condition is to first reduce the spender’s allowance to 0 and set the desired value afterwards: https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729"

The link: https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-approve-address-uint256-

Also, check the returned value of the external call approveTo.