search

code-423n4 / 2022-06-illuminate-findings

1 stars 0 forks source link

ERC5095 redeem/withdraw does not update allowances #245

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-illuminate/blob/main/marketplace/ERC5095.sol#L100

Vulnerability details

ERC5095's redeem/withdraw allows an ERC20-approved account to redeem user's tokens, but does not update the allowance after burning.

Impact

User Mal can burn more tokens than Alice allowed him to. He can set himself to be the receiver of the underlying, therefore Alice will lose funds.

Proof of Concept

withdraw and redeem functions check that the msg.sender has enough approvals to redeem the tokens:

            require(_allowance[holder][msg.sender] >= underlyingAmount, 'not enough approvals');

But they do not update the allowances. They then call authRedeem, which also does not update the allowances. Therefore, an approved user could "re-use his approval" again and again and redeem whole of approver's funds to himself.

Recommended Mitigation Steps

Update the allowances upon spending.

KenzoAgada commented 2 years ago

(Although this is my issue I am referring dups to it as I find it has the best descriptive title and brevity for final report and right mitigation unlike 174. Obviously judge is free to change it later.)

sourabhmarathe commented 2 years ago

While we did not actually intend to audit the 5095 itself, as 5095 itself is not yet final, we did describe its purpose in our codebase in the initial readme, and didn't specify that it was not in scope.

With that context, we will leave it up to the judges whether or not to accept issues related to the ERC5095 token.