SingleJoin/SingleExit contracts use transfer() to send ether back to the caller

[code423n4]

Handle

Ruhum

Vulnerability details

Impact

The SingleJoin & SingleExit contracts use transfer() to send ether to the caller. If the caller is a smart contract the call might fail reverting the transaction if:

• caller contract isn't able to receive ether (missing receive() or payable fallback() function)
• caller contract uses more than the provided 2300 gas when receiving ether

Since gas costs might increase in the future, using transfer() might cause these contracts to potentially break since transfer() would always fail.

Here's a blogpost about using transfer(): https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

I'd rate the issue as medium since a part of the protocol would potentially be unusable.

Proof of Concept

Usage of transfer():

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoin.sol#L37

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/EthSingleTokenJoinV2.sol#L37

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExit.sol#L93

https://github.com/code-423n4/2021-12-amun/blob/main/contracts/basket/contracts/singleJoinExit/SingleNativeTokenExitV2.sol#L121

Tools Used

none

Recommended Mitigation Steps

Instead, the contracts should use:

(bool success, ) = payable(msg.sender).call{value: intermediateTokenBalance}("");
require(success);

Note that this approach introduces a risk of reentrancy. But, that shouldn't be a problem for these specific contracts. Nevertheless, it might be worth adding reentrancy guards in case the contracts ever get extended.

[loki-sama]

duplicate #175