Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DPR-01M] Inexistent Revocation of Administrators #15

Closed
galimba opened this issue Nov 6, 2024 · 2 comments · Fixed by #35
Closed

[DPR-01M] Inexistent Revocation of Administrators #15

galimba opened this issue Nov 6, 2024 · 2 comments · Fixed by #35
Assignees
Labels
AUDIT To be fixed and reviewed ASAP review needed

Comments

@galimba
Copy link
Contributor

galimba commented Nov 6, 2024

DPR-01M: Inexistent Revocation of Administrators

Type Severity Location
Logical Fault DataPointRegistry.sol:L47-L54

Description:

The DataPointRegistry::transferOwnership function permits a particular DataPoint to be transferred to a new owner, however, the DataPoint will retain all original authorized administrators.

This represents a flaw in the transfer process as the recipient of the DataPoint cannot react during its acceptance, requiring two separate blockchain blocks for a new owner to remove the administrators they wish.

Impact:

The DataPointRegistry::transferOwnership function necessitates two distinct interactions across two different blocks for a transfer to be accompanied with administrator removal which we consider an approach prone to errors and exploitation.

Example:

/// @inheritdoc IDataPointRegistry
function transferOwnership(DataPoint dp, address newOwner) external {
    DPAccessData storage dpd = _accessData[dp];
    address currentOwner = dpd.owner;
    if (msg.sender != currentOwner) revert InvalidDataPointOwner(dp, msg.sender);
    dpd.owner = newOwner;
    emit DataPointOwnershipTransferred(dp, currentOwner, newOwner);
}

Recommendation:

We advise the system to revoke all previous administrators during a transfer by using a nonce system or a similar approach, ensuring that the DataPoint is transferred in a fresh state and a rogue administrator cannot affect it after it has exchanged hands.

@galimba galimba added the AUDIT To be fixed and reviewed ASAP label Nov 6, 2024
@Berbex Berbex self-assigned this Nov 6, 2024
@Berbex
Copy link
Collaborator

Berbex commented Nov 6, 2024

We talked about the way to solve this, but since there isn't EnumerableMap.AddressToBool mapping, I went to the solution using EnumerableSet.AddressSet where if the address is contained is the True of the previous idea, but according to ethereum/solidity/pull/11843[https://github.com/ethereum/solidity/pull/11843] we cannot use delete to clean the set, a possible solution is still using the set idea but adding a clean() internal function that remove all the elements of an array one by one, but I want to know your opinions @pash7ka @galimba

@Berbex Berbex added the question Further information is requested label Nov 6, 2024
@pash7ka
Copy link
Collaborator

pash7ka commented Nov 7, 2024

Yes, using EnumerableSet.AddressSet is good here. For cleaning this Set we will need a loop, but i see no other way.

@Berbex Berbex added review needed and removed question Further information is requested labels Nov 7, 2024
@Berbex Berbex linked a pull request Nov 7, 2024 that will close this issue
@Berbex Berbex closed this as completed in #35 Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AUDIT To be fixed and reviewed ASAP review needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants