Sparkn

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

Lack of checking the existence of the Proxy contract

serialcoder

Summary

If the ProxyFactory::distributeByOwner() is executed before the Proxy contract has been deployed, the transaction will be executed successfully, but the stuck tokens will not be transferred to a rescue requestor.

Vulnerability Details

The distributeByOwner() is used for recovering the rescue requestor's stuck tokens after a contest expires. The function will trigger the _distribute() to execute the contest's Proxy contract.

The transaction will not be reverted as expected if the Proxy has not been deployed before calling the distributeByOwner(). In other words, the transaction will be executed successfully, but the stuck tokens will not be transferred to the rescue requestor.

The root cause is that the _distribute() will make a low-level call to the Proxy without checking the existence of its contract.

function distributeByOwner(
address proxy,
address organizer,
bytes32 contestId,
address implementation,
bytes calldata data
) public onlyOwner {
if (proxy == address(0)) revert ProxyFactory__ProxyAddressCannotBeZero();
bytes32 salt = _calculateSalt(organizer, contestId, implementation);
if (saltToCloseTime[salt] == 0) revert ProxyFactory__ContestIsNotRegistered();
// distribute only when it exists and expired
if (saltToCloseTime[salt] + EXPIRATION_TIME > block.timestamp) revert ProxyFactory__ContestIsNotExpired();
@> _distribute(proxy, data);
}
​
...
​
function _distribute(address proxy, bytes calldata data) internal {
@> (bool success,) = proxy.call(data);
if (!success) revert ProxyFactory__DelegateCallFailed();
@> emit Distributed(proxy, data);
}

  • The distributeByOwner() triggers the _distribute(): https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L217

  • The _distribute() makes a low-level call without checking the existence of the Proxy's contract: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L250

  • The event Distributed will be emitted regardless of whether the Proxy has a contract: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L252

Impact

The rescue transaction will be executed successfully, but the stuck tokens will not be transferred to the rescue requestor. This incident can cause off-chain services to malfunction and confuse the owner (protocol admin) and the rescue requestor.

Tools Used

Manual Review

Recommendations

Verify that the target Proxy's address has a contract bytecode before executing the Proxy in the _distribute(), as below.

+ import {Address} from "openzeppelin/utils/Address.sol";
​
...
​
function _distribute(address proxy, bytes calldata data) internal {
+ if (!Address.isContract(proxy)) revert ProxyFactory__NoProxyContract();
(bool success,) = proxy.call(data);
if (!success) revert ProxyFactory__DelegateCallFailed();
emit Distributed(proxy, data);
}

Support

FAQs

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