Sparkn

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

The distributeByOwner() can mistakenly drain all tokens from an incorrect Proxy

serialcoder

Summary

The ProxyFactory::distributeByOwner() can unexpectedly drain all tokens from an incorrect contest's Proxy contract. The affected contest can be under any organizer and does not need to expire before.

Vulnerability Details

The distributeByOwner() receives the proxy (in L206) and other input parameters. The function will compute a salt from the organizer, contestId, and implementation parameters and then validate that the contest corresponding to the computed salt must expire before executing the _distribute().

Apparently, the distributeByOwner() does not execute the _distribute() by passing the Proxy address derived from the previously computed salt. Specifically, the inputted proxy parameter will be passed instead.

If the proxy parameter is mistakenly inputted, the proxy parameter can point to another contest's Proxy 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);
}

  • The proxy param is inputted by an owner: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L206

  • The proxy param can point to an incorrect Proxy contract: https://github.com/Cyfrin/2023-08-sparkn/blob/0f139b2dc53905700dd29a01451b330f829653e9/src/ProxyFactory.sol#L217

Impact

The distributeByOwner() can unexpectedly drain all tokens from an incorrect contest's Proxy contract. The affected contest can be under any organizer and does not need to expire before (but must be closed already).

To clarify the vulnerability, although only an owner can execute the distributeByOwner() and the owner is trusted, the incident can occur by mistake (i.e., this vulnerability is not about any centralization or trust risks; it is about the risks of input mistakes only).

The likelihood is considered LOW (since the owner is expected to do due diligence). The impact is considered HIGH. Therefore, the severity is considered MEDIUM.

Tools Used

Manual Review

Recommendations

Derive the Proxy address from the computed salt by consulting the getProxyAddress(salt, implementation), as shown below.

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();
+ address proxy = getProxyAddress(salt, implementation);
_distribute(proxy, data);
}

Support

FAQs

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