Sparkn

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

Lack of contract existence check on delegatecall will result in unexpected behavior

2997ms

Summary

The Proxy contract uses the delegatecall proxy pattern. If the implementation contract is incorrectly set or is self-destructed, the Proxy contract may not be able to detect failed executions. This fallback function lacks a contract existence check. Also it lacks the check that if the address of implementation might be 0 before executing.

Vulnerability Details

https://github.com/Cyfrin/2023-08-sparkn/blob/main/src/Proxy.sol#L51-L63

fallback() external {
address implementation = _implementation;
assembly {
let ptr := mload(0x40)
calldatacopy(ptr, 0, calldatasize())
let result := delegatecall(gas(), implementation, ptr, calldatasize(), 0, 0)
let size := returndatasize()
returndatacopy(ptr, 0, size)
​
switch result
case 0 { revert(ptr, size) }
default { return(ptr, size) }
}
}
​

We need to check if the implementation address is 0 or not before executing the next steps.
Also we don't have a contract existence check for this address.

Impact

A delegatecall to a destructed contract will return success. Due to the lack of contract existence checks, a series of batched transactions may appear to be successful even if one of the transactions fails.

See the solidity documentation here: https://docs.soliditylang.org/en/develop/control-structures.html#error-handling-assert-require-revert-and-exceptions

The low-level functions call, delegatecall and staticcall return true as their first return value if the account called is non-existent, as part of the design of the EVM. Account existence must be checked prior to calling if needed.

Tools Used

Manual review

Recommendations

  • Implement a contract existence check before each delegatecall.

  • Add a require(implementation != address(0)) before executing

Support

FAQs

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