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

Penalty value lost if penaltyForwarder is address(0) #95

Open
code423n4 opened this issue May 17, 2022 · 2 comments
Open

Penalty value lost if penaltyForwarder is address(0) #95

code423n4 opened this issue May 17, 2022 · 2 comments
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraMerkleDrop.sol#L151-L152

Vulnerability details

Impact

Penalty details are lost if the penaltyForwarder address is address(0)

Proof of Concept

https://github.com/code-423n4/2022-05-aura/blob/main/contracts/AuraMerkleDrop.sol#L151-L152

Here the check for address(0) is used after the penalty is set to zero. If the require statement fails in this function, the accumulated penalty still remains set to zero.

Tools Used

Manual checks

Recommended Mitigation Steps

Add the address(0) check before setting the penalty as zero

require(penaltyForwarder != address(0), "!forwarder");
aura.safeTransfer(penaltyForwarder, toForward);
pendingPenalty = 0;

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels May 17, 2022
code423n4 added a commit that referenced this issue May 17, 2022
@0xMaharishi 0xMaharishi added disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons labels May 25, 2022
@0xMaharishi
Copy link

technically true but arguable invalid.. this won't be set in the constructor to be 0

@0xMaharishi 0xMaharishi added the resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) label May 30, 2022
@dmvt
Copy link
Collaborator

dmvt commented Jun 22, 2022

I'm leaving this one in place as a gas issue. There is no way to update penaltyForwarder meaning this mistake would be irreversible and require a redeployment. Beyond that there is no impact.

@dmvt dmvt added G (Gas Optimization) and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) G (Gas Optimization) resolved Finding has been patched by sponsor (sponsor pls link to PR containing fix) sponsor acknowledged Technically the issue is correct, but we're not going to resolve it for XYZ reasons
Projects
None yet
Development

No branches or pull requests

3 participants