certik针对pancakeswap的审计报告学习

审计报告总结

1. 共发现9个问题，其中5个是information级别，其他更严重级别的问题pancakeswap团队已经修复或者跟certik沟通表示没有影响
2. 通过审计报告看项目还是很健康的。

审计项目地址

https://www.certik.org/projects/pancakeswap

审计发现的问题

暂时忽略代码格式之类的问题，主要列出中级以上的问题。

SBR-01: Incorrect Delegation Flow

问题比较简单直接看修复提交的代码：在burn时需要把此账户的授权的地址修改为0地址，其实就是把授权移除；这个问题应该是个手误。

commit

SCF-01 addressList Inaccuracy

Description:

The first linked if block pushes a new address to the addressList array in the case the userInfo mapping lookup yields 0 on the amount member. This case is possible even after the user has already been added to the array, either by invoking emergencyWithdraw or withdrawing the full amount held by the user.

Recommendation:

We advise that the push mechanism is revised to ensure that the user does not already exist in the array.

Alleviation:

The PancakeSwap team altered the condition for pushing new items to the addressList array, however duplicates can still exist. After conversing with the team, we were informed that the array is not utilized on-chain and is meant to aid off-chain processes in an airdrop mechanism which will eliminate duplicate addresses. As such, this issue can be safely ignored. We would like to note that this is not an optimal mechanism to conduct this, as it would be better to instead rely on emitted events and blockchain analysis rather than contract storage

修复后的代码如下：

certik提出的问题意思是，在存款时通过用户的amount是否为0判断地址是否存在addressList中不严谨，因为可能之前存过一次然后全部提现了，这样就会导致amount为0并且在addressList中存在。

pancakeswap团队通过遍历addressList来判断地址是否存在于addressList中。

commit

个人理解

通过遍历addressList的方式来查找某个地址是否在这个列表中不太可取，因为这增加了gas费。

如果别人想要攻击一直存入很小的amount来增加addressList的大小会导致存款的方法的gas费特别高。

SCF-03 Incorrect Reset Mechanism

Description:

The emergencyWithdraw function is meant to “reset” a user’s state and withdraw his deposited tokens. In this case, the rewardPending variable of the user struct is not zeroed out.

Recommendation:

As the rewardPending member is cumulative, it is possible to exploit this behavior and artificially increase the pending rewards of a user. We advise that either a manual 0 assignment statement is introduced in the emergencyWithdraw function or a delete operation is conducted on the full struct located at userInfo[msg.sender].

Alleviation:

The emergencyWithdraw function was properly fixed to zero out all members of the UserInfo struct

修复后代码如下

此问题是说在转账之后应该吧user结构里面的值都更新为0，团队已经做了修复。

// Withdraw without caring about rewards. EMERGENCY ONLY.
function emergencyWithdraw() public {
    UserInfo storage user = userInfo[msg.sender];
    syrup.safeTransfer(address(msg.sender), user.amount);
    emit EmergencyWithdraw(msg.sender, user.amount);
    user.amount = 0;
    user.rewardDebt = 0;
    user.rewardPending = 0; // 此行为修复行
}

个人理解

最好是先修改user结构中的值在进行转账，这样应该是最佳开发实践。