code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/Funding.sol#L52-L66

Vulnerability details

Impact

In Funding.sol contract, _execute() function is as shown as below,

File: ajna-grants/src/grants/base/Funding.sol

52    function _execute(
53        uint256 proposalId_,
54        address[] memory targets_,
55        uint256[] memory values_,
56        bytes[] memory calldatas_
57    ) internal {
58        // use common event name to maintain consistency with tally
59        emit ProposalExecuted(proposalId_);
60
61        string memory errorMessage = "Governor: call reverted without message";
62        for (uint256 i = 0; i < targets_.length; ++i) {
63            (bool success, bytes memory returndata) = targets_[i].call{value: values_[i]}(calldatas_[i]);
64            Address.verifyCallResult(success, returndata, errorMessage);
65        }
66    }

This function is utilized in a few different places in the contracts like in ExtraordinaryFunding.sol and StandardFunding.sol

According to the Solidity documentation, “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”.

Link to solidity reference

As per above _execute( ) function does not check target address is a contract address or Externally owned account address(EOA) so as a result, it is possible that the .call will fail, but the function using the above low level .call will not notice anything went wrong. In particular, it is possible that the address like target address is a deleted contract or non-existent contract, but the functions will not revert. The .call( ) here will return success even if the target address doesn’t exist at any incorrect address. Therefore, low level functions i.e .call() may fail silently.

To reconfirmed and reassured the target address must be the contract address, check below comments from contract(L-48),

File: ajna-grants/src/grants/base/Funding.sol

46     /**
47     * @notice Execute the calldata of a passed proposal.
48     * @param targets_   The list of smart contract targets for the calldata execution. Should be the Ajna 
        token address.
49     * @param values_    Unused. Should be 0 since all calldata is executed on the Ajna token's transfer 
          method.
50     * @param calldatas_ The list of calldatas to execute.
51     */
52    function _execute(
53        uint256 proposalId_,

As seen above the target address must be the contract address which should be Ajna token address. Therefore as per solidity technical documentation, It would be better to check for the contract’s existence prior to calling/executing low level .call( ) function.

Proof of Concept

In Funding.sol contract,

https://github.com/code-423n4/2023-05-ajna/blob/276942bc2f97488d07b887c8edceaaab7a5c3964/ajna-grants/src/grants/base/Funding.sol#L52-L66

In above smart contract link, See _execute() here. Please check how this function is called with address target, but this "address target" contract’s existence is not verified and as per contract comment it must be a contract address(Ajan token address) so that validation is missing here and this problem as described above in detail.

References for similar low level functions contract existent issues i.e “call”, “delegatecall”, staticall can be checked here for reference information,

1)Reference link for closely similar finding in Amun audit by code4rena- Link to audit reference

2)Reference link for closely similar finding in Trader Joe audit by code4rena- Link to audit reference

Tools Used

Manual Analysis

Recommended Mitigation Steps

1)Check for contract existence on low-level calls, so that failures are not missed.

2)Alternatively, It is recommended to use Openzeppelin Address.isContract () function which will make sure to check that contract actually exists before calling it by .call( )

Reference link for Address.isContract( )

3)Add address(0) check validation and ensure target address is not zero address.

Assessed type

call/delegatecall

Picodes commented 1 year ago

True but it would only mean that the proposal was useless

c4-judge commented 1 year ago

Picodes marked the issue as duplicate of #253