DelegateToken.approve will override the old approval with the same delegateId. This will directly impact the rights of users who were previously approved, effectively revoking their previous approval.
There three users: from, to and charle.
from creates a delegateId and approves it to to, then approves it to charle.
Now, to and charle should be able to operate on this delegateId. However, to's authorization has been overridden by charle and to can no longer transfer this delegateId.
Add the test to test/DelegateToken.t.sol, then run it with forge test --match-test testApprove -vvv.
Failing tests:
Encountered 1 failing test in test/DelegateToken.t.sol:DelegateTokenTest
[FAIL. Reason: NotApproved(0x0000000000000000000000000000000000000001, 83125656321449047896925968989594772657464967156116746192883976214766562556269 [8.312e76])] testApprove() (gas: 453013)
Tools Used
forge
Recommended Mitigation Steps
Maybe the approval for a single delegateId should also be stored in a mapping like accountOperator, rather than stored in the DelegateTokenStorageHelpers.
Lines of code
https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenStorageHelpers.sol#L39 https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenStorageHelpers.sol#L39
Vulnerability details
Impact
DelegateToken.approve
will override the old approval with the samedelegateId
. This will directly impact the rights of users who were previously approved, effectively revoking their previous approval.Proof of Concept
There three users: from, to and charle. from creates a delegateId and approves it to to, then approves it to charle. Now, to and charle should be able to operate on this
delegateId
. However, to's authorization has been overridden by charle and to can no longer transfer this delegateId.Add the test to
test/DelegateToken.t.sol
, then run it withforge test --match-test testApprove -vvv
.Result:
Tools Used
forge
Recommended Mitigation Steps
Maybe the approval for a single
delegateId
should also be stored in a mapping likeaccountOperator
, rather than stored in theDelegateTokenStorageHelpers
.Assessed type
Access Control