search

re-al-Foundation / rwa-contracts

Core dev environment for the RWA Governance contracts
0 stars 0 forks source link

[DFY-02M] Delegator Removal Re-Entrancy Flaw #29

Closed chasebrownn closed 7 months ago

chasebrownn commented 7 months ago

DFY-02M: Delegator Removal Re-Entrancy Flaw

Type Severity Location
Language Specific DelegateFactory.sol:L102-L130, L136-L162

Description:

The DelegateFactory::revokeExpiredDelegators function will iterate through the delegators in the system and attempt to withdraw the delegated token for each if they have expired.

The problem lies in the fact that the Delegator::withdrawDelegatedToken function will transfer the EIP-721 asset using the IERC721::safeTransferFrom function which will in turn inform the recipient.

The recipient can then:

The above will result in the delegators array being reduced by one on each re-entry incorrectly for the same entry. To avoid out-of-bound access errors, the user can also re-enter the DelegateFactory::deployDelegator function resulting in their newly deposited token overwriting the previously-last entry in the delegators array and thus causing the NFT to be lost.

Impact:

It is presently possible to cause the last entry in the delegators array to be erased incorrectly via a re-entrancy attack, thereby causing it to be lost. The exhibit requires the malicious user to be able to re-enter the DelegateFactory::deployDelegator function thereby reducing the finding's severity to medium.

Example:

// call withdrawDelegatedToken
(bool success,) = delegator.call(abi.encodeWithSignature("withdrawDelegatedToken()"));
require(success, "withdraw unsuccessful");

// delete delegator from contract
delete isDelegator[delegator];
delete delegatorExpiration[delegator];
delegators[i] = delegators[length - 1];
delegators.pop();
--length;

emit DelegatorDeleted(delegator);

Recommendation:

We advise the Delegator::withdrawDelegatedToken function to utilize a normal IERC721::transferFrom operation, and the DelegateFactory::deployDelegator and DelegateFactory::revokeExpiredDelegators functions to be non-reentrant preventing the misbehaviour described.

chasebrownn commented 7 months ago

Resolved