Sparkn

CodeFox Inc.
DeFiFoundryProxy
15,000 USDC
View results
Previous Next
Submission Details
Severity: medium

Proxy contract fallback cant receive ether

0x4non

Summary

The Proxy contract's fallback function does not support receiving ether. This could lead to potential issues if paired with an implementation contract that is intended to receive ether. Core contract implementation could be brick or non functional at all. Please review the current suggestion and the suggest to the test testIfSendEtherToProxyThenRevert

Vulnerability Details

The Proxy contract's fallback function it lacks the payable keyword, which means it cannot receive ether. If the implementation is doing some defi and needs to receive ether the proxy contract will reject the transaction. This could be problematic if the _implementation contract is designed to accept ether.

Impact

Functionality Limitation: The proxy contract cannot be paired with any implementation contract that requires the reception of ether, limiting its usability.

Tools Used

Manual review.

Recommendations

  1. Add the payable keyword to the fallback function in the Proxy contract. This will allow the proxy to receive ether and delegate it to the _implementation contract if necessary.

  2. Alternatively, consider using OpenZeppelin's proxy contract which is well-tested and widely adopted in the community. It already handles such scenarios and ensures that the proxy can receive ether if the implementation contract is designed to do so.

Would also love to add a few gas optimizations:

diff --git a/src/Proxy.sol b/src/Proxy.sol
index 516df7d..e71f00e 100644
--- a/src/Proxy.sol
+++ b/src/Proxy.sol
@@ -48,18 +48,17 @@ contract Proxy {
/**
* @dev Delegate all the calls to implementation contract
*/
- fallback() external {
+ fallback() external payable {
address implementation = _implementation;
assembly {
- let ptr := mload(0x40)
- calldatacopy(ptr, 0, calldatasize())
- let result := delegatecall(gas(), implementation, ptr, calldatasize(), 0, 0)
+ calldatacopy(0, 0, calldatasize())
+ let result := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0)
let size := returndatasize()
- returndatacopy(ptr, 0, size)
+ returndatacopy(0, 0, size)
switch result
- case 0 { revert(ptr, size) }
- default { return(ptr, size) }
+ case 0 { revert(0, size) }
+ default { return(0, size) }
}
}
}

This is the recommendation on the current test suite:

diff --git a/test/uint/OnlyProxyTest.t.sol b/test/uint/OnlyProxyTest.t.sol
index 6f8d72e..b775411 100644
--- a/test/uint/OnlyProxyTest.t.sol
+++ b/test/uint/OnlyProxyTest.t.sol
@@ -43,13 +43,20 @@ contract ProxyTest is StdCheats, Test {
function testIfSendEtherToProxyThenRevert() public {
vm.deal(msg.sender, 2 ether);
- vm.expectRevert();
(bool success,) = address(proxy).call{value: 1 ether}("");
// no ether arrived
- assertEq(0, address(proxy).balance);
+ assertEq(1 ether, address(proxy).balance);
+ assertEq(success, true);
+
+ Proxy proxyReject = new Proxy(address(new Foo()));
- (bool success2,) = address(secondProxy).call{value: 1 ether}("");
+ vm.expectRevert();
+ (success,) = address(proxyReject).call{value: 1 ether}("");
// no ether arrived
- assertEq(0, address(secondProxy).balance);
+ assertEq(0, address(proxyReject).balance);
}
}
+
+// this contract has no receive function, so it will reject ether
+contract Foo {}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!