Summary
The issue was found in the Starklane smart contract under the withdrawTokens function, where the function is marked as payable but does not correctly handle cases where ETH is sent to it. This could potentially lead to unexpected behaviors or the loss of funds if ETH is sent without proper handling.
Vulnerability Details
The vulnerability stems from the fact that the withdrawTokens function is marked as payable, allowing ETH to be sent along with the transaction. However, the contract does not have mechanisms in place to utilize, refund, or explicitly reject the ETH, which means it can accumulate in the contract without any intended purpose.
Impact
A user could accidentally send ETH, which would lead to the funds being effectively locked and unrecoverable.
Proof of Code:
Step 1) Set up the testing contract:
pragma solidity ^0.8.0;
import "openzeppelin-contracts/contracts/token/ERC721/IERC721.sol";
import "openzeppelin-contracts/contracts/token/ERC1155/IERC1155.sol";
import "forge-std/Test.sol";
import "../src/IStarklane.sol";
import "../src/IStarklaneEvent.sol";
import "../src/Bridge.sol";
import "../src/sn/Cairo.sol";
import "../src/sn/StarknetMessagingLocal.sol";
import "../src/sn/IStarknetMessagingLocal.sol";
import "../src/token/Deployer.sol";
import "../src/token/TokenUtil.sol";
import "./utils/Users.sol";
import "./token/ERC721MintFree.sol";
import "./token/IERC721MintRangeFree.sol";
* @title Bridge testing.
*/
contract BridgeTest is Test, IStarklaneEvent {
address bridge;
address erc721C1;
address erc1155C1;
address snCore;
address payable internal alice;
address payable internal bob;
function setUp() public {
Users genusers = new Users();
address payable[] memory users = genusers.create(5);
alice = users[0];
bob = users[1];
erc721C1 = address(new ERC721MintFree("name 1", "S1"));
snCore = address(new StarknetMessagingLocal());
address impl = address(new Starklane());
bytes memory dataInit =
abi.encodeWithSelector(Starklane.initialize.selector, abi.encode(address(this), snCore, 0x1, 0x2));
bridge = address(new ERC1967Proxy(impl, dataInit));
IStarklane(bridge).enableBridge(true);
}
Step 2) Set up the function to test. The function will send ETH, effectively locking it away:
This function tests that ETH can be sent to the bridge thru the withdrawTokens
function and therefore locked in the bridge.
*/
function test_withdrawTokensAndLockETH() public {
felt252 header = Protocol.requestHeaderV1(CollectionType.ERC721, false, false);
Request memory req = buildRequestDeploy(header, 888, bob);
uint256[] memory reqSerialized = Protocol.requestSerialize(req);
bytes32 msgHash = computeMessageHashFromL2(reqSerialized);
uint256[] memory hashes = new uint256[](1);
hashes[0] = uint256(msgHash);
IStarknetMessagingLocal(snCore).addMessageHashesFromL2(hashes);
uint256 withdrawAmount = 1 ether;
vm.deal(bob, withdrawAmount);
vm.prank(bob);
address collection = IStarklane(bridge).withdrawTokens{value: 1 ether}(reqSerialized);
assertEq(IERC721(collection).ownerOf(888), bob);
uint256 bridgeBalance = address(bridge).balance;
console.log("Bridge balance: %s", bridgeBalance);
assertEq(bridgeBalance, withdrawAmount, "ETH was not locked in the bridge");
}
Tools Used
Manual review
Recommendations
-
If the contract is not intended to receive ETH, the payable modifier should be removed from the withdrawTokens function.
-
Implement proper logic to either refund the user or add a withdrawETH function.
function withdrawTokens(...) {
if (msg.value > 0) {
(bool success, ) = msg.sender.call{value: msg.value}("");
require(success, "ETH transfer failed");
}
}
function withdrawETH() external onlyOwner {
(bool success, ) = msg.sender.call{value: address(this).balance}("");
require(success, "ETH transfer failed");
}
