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

Potentially Dangerous Function Input #129

Closed
aaitor opened this issue Aug 9, 2021 · 0 comments · Fixed by #145
Closed

Potentially Dangerous Function Input #129

aaitor opened this issue Aug 9, 2021 · 0 comments · Fixed by #145
Assignees
Labels
bug Something isn't working severity:minor

Comments

@aaitor
Copy link
Member

aaitor commented Aug 9, 2021

NFC-01M: Potentially Dangerous Function Input

Type Severity Location
Input Sanitization Minor NFTAccessCondition.sol:L141

Description:

The _contractAddress input variable of fulfill remains undocumented and as such renders difficult the validation of what level of input sanitization should be applied to it.

Example:

**
* @notice fulfill NFT Access condition
* @dev only DID owner or DID provider can call this
*       method. Fulfill method sets the permissions 
*       for the granted consumer's address to true then
*       fulfill the condition
* @param _agreementId agreement identifier
* @param _documentId refers to the DID in which secret store will issue the decryption keys
* @param _grantee is the address of the granted user or the DID provider
* @return condition state (Fulfilled/Aborted)
*/
function fulfill(
    bytes32 _agreementId,
    bytes32 _documentId,
    address _grantee,
    address _contractAddress
)

Recommendation:

We advise it to be documented in the comments that precede the function and if the value is crucial to prevent users arbitrarily setting it as the fulfill function with 4 arguments is available as public and can set an arbitrary contract address in place of the DID registry.

@aaitor aaitor added bug Something isn't working severity:minor labels Aug 9, 2021
@aaitor aaitor added this to the NFT 721 Security Issues milestone Aug 9, 2021
@aaitor aaitor self-assigned this Aug 9, 2021
@aaitor aaitor linked a pull request Aug 9, 2021 that will close this issue
aaitor added a commit that referenced this issue Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working severity:minor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant