code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L247-L258

Vulnerability details

Impact

An unexpected return value from the EIP1271 signature verification can lead to authorization for unwanted operations in external protocols.

Proof of Concept

As shown in the EIP1271 standard specification, when a signature is not valid, the returned magic value is 0xffffffff:

  /**
   * @notice Verifies that the signer is the owner of the signing contract.
   */
  function isValidSignature(
    bytes32 _hash,
    bytes calldata _signature
  ) external override view returns (bytes4) {
    // Validate signatures
    if (recoverSigner(_hash, _signature) == owner) {
      return 0x1626ba7e;
    } else {
      return 0xffffffff;
    }
  }

In the DAO contract, a signatureValidator smart contract is responsible for determining the validity of a given signature. However, when signatureValidator is not set, the signature is considered invalid, and a magic number of 0x00000000 is returned instead of the one from the standard:

  function isValidSignature(
      ...
      if (address(signatureValidator) == address(0)) {
         // Return the invalid magic number
         return bytes4(0);
      }
      ...
  }

Consider the case where an external protocol that has to validate a given signature performs the following check:

 function withdrawFrom(address from, bytes calldata signature) external {
     ...
     // validate `from` address signature
     if (IERC1271(from).isValidSignature(allowanceHash, signature) != 0xffffffff) {
         revert WithdrawalNotAllowed();
     }
     // withdraw from `from` address
     ...
 }

In the above case, if the externally passed from address is the address of the DAO and there is no signatureValidator set, the returned magic value will be 0x00000000 while it is expected to be 0xffffffff. This will break the signature verification mechanism and allow execution of any arbitrary operation in the external contract, possibly leading to stolen funds or just unexpected authorization of arbitrary operations.

Tools Used

Manual review

Recommended Mitigation Steps

Return the correct 'invalid magic number' when signatureValidator is not set:

  247:    /// @inheritdoc IDAO
  248:    function isValidSignature(
  249:        bytes32 _hash,
  250:        bytes memory _signature
  251:    ) external view override(IDAO, IERC1271) returns (bytes4) {
  252:        if (address(signatureValidator) == address(0)) {
  253:            // Return the invalid magic number
- 254:            return bytes4(0);
+ 255:            return 0xffffffff;
  255:        }
  256:        // Forward the call to the set signature validator contract
  257:        return signatureValidator.isValidSignature(_hash, _signature);
  258:    }

0xean commented 1 year ago

warden should re-review https://eips.ethereum.org/EIPS/eip-1271

and also note the reference implementation

  function callERC1271isValidSignature(
    address _addr,
    bytes32 _hash,
    bytes calldata _signature
  ) external view {
    bytes4 result = IERC1271Wallet(_addr).isValidSignature(_hash, _signature);
    require(result == 0x1626ba7e, "INVALID_SIGNATURE");
  }

c4-judge commented 1 year ago

0xean marked the issue as unsatisfactory: Invalid

code-423n4 / 2023-03-aragon-findings