code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/36477d96788791ff07a1ba40d0c726fb39bf05ec/src/vault/AdminProxy.sol#L15-L26

Vulnerability details

Impact

AdminProxy is the hot spot for all low-level calls, therefore it should do some extra security checks that are currently not in place.

By design a Solidity low level call to a zero address or an EOA (non contract) address will return success true. The only way to detect whether the success = true is actually indicating the call succeeded is by checking the return data length:

if greater than zero it is definitely a successfull call to a contract function
if zero it must be discriminated

To discriminate in the latter case, the presumed contract address need to be checkd for code length.

Proof of Concept

https://github.com/code-423n4/2023-01-popcorn/blob/36477d96788791ff07a1ba40d0c726fb39bf05ec/src/vault/AdminProxy.sol#L15-L26

Tools Used

n/a

Recommended Mitigation Steps

The AdminProxy could be changed as a minimum as follows:

contract AdminProxy is Owned {
  constructor(address _owner) Owned(_owner) {}

  /// @notice Execute arbitrary management functions.
  function execute(address target, bytes calldata callData)
    external
    onlyOwner
    returns (bool success, bytes memory returndata)
  {
    (success, returnData) = target.call(callData);
    if(success == true && returndata.length == 0) success = target.code.size > 0;
  }
}

c4-sponsor commented 1 year ago

RedVeil marked the issue as sponsor disputed

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-01-popcorn-findings